Skip to content

Conversation

morenonicolelli
Copy link

@morenonicolelli morenonicolelli commented Aug 11, 2025

What's changed

Make the package more Swifty and make ti easy to use and maintain.

Move all different internal package into the main one. I believe this setup and structure help future contributors working better and easier, because multiple packages into one is difficult to open and work on Xcode.
This is the first step, it's needed an update to all scripts and CI/CD files. In this moment I don't understand everything but I'm open to help and contribute.

Closes #78.

@kou
Copy link
Member

kou commented Aug 11, 2025

Could you open an issue for this and describe what is "Swifty" (including documentation URL is better) in the issue?
See also: https://github.com/apache/arrow-swift/blob/main/CONTRIBUTING.md

We'll refer the issue in this PR description.

@morenonicolelli
Copy link
Author

Hi!!
Thanks for the quick answer. I open an issue as you request.

#78

I hope is well done.

@abandy
Copy link
Contributor

abandy commented Aug 17, 2025

lgtm!

@kou kou changed the title Swifty Package chore: Improve folder structure Aug 18, 2025
@kou
Copy link
Member

kou commented Aug 18, 2025

Could you fix lint failures by pre-commit run --show-diff-on-failure --color=always --all-files?

This package structure more swifty than previous one. With this structure testing and building is easier and it's no more needed scripts
Migrated as executable target. This makes the target executable from the command line using swift run go-swift.
The idea is to make a pre-build plugin and use it before test launch
Copy link

@willtemperley willtemperley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's looking so much better than before! Now I can build and run it in Xcode, although I needed to change the go target to a plain target to make it build.

@morenonicolelli
Copy link
Author

@willtemperley I'm struggling with CI/CD. if fixed we can move to other improvements and if you want I can help. in my opinion test have to be make during swift test not during CI/CD phase to improve testability and development phase.

Package.swift Outdated
// build: .unsafeFlags(["-warnings-as-errors"])
]
),
.library(

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs to be a .target

@willtemperley
Copy link

@willtemperley I'm struggling with CI/CD. if fixed we can move to other improvements and if you want I can help. in my opinion test have to be make during swift test not during CI/CD phase to improve testability and development phase.

@morenonicolelli Yes I agree. I think the CI failure is happening during the test file generation, I'd say just delete that part and check in the test files. I've opened an issue for that: #82

…ly. Removed data generation part of build script.
Copy link

@willtemperley willtemperley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good except the tests. I opened a pull request on your fork with the test data added and all test going green.

I also removed the now unnecessary part of the build script which was failing.

I think if you want to merge that, then that should arrive here.

Generated and added test data. All tests now green when running local…
Copy link

@willtemperley willtemperley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Should pass CI

@kou
Copy link
Member

kou commented Aug 30, 2025

Please don't mix #82 and this.
If we need to fix #82 for this, let's work on #82 before this.
We can restart this after #82 is completed.

@morenonicolelli
Copy link
Author

in my opinion this two issues are strictly linked. you can't resolve tests with the old structure and you can't close new structure with the old tests

@willtemperley
Copy link

Yes, before #77 it was impossible to test locally. The work @morenonicolelli did enabled me to run the tests in a standard Swift testing environment, so I was able to refactor them.

@kou
Copy link
Member

kou commented Aug 30, 2025

Why? We can generate test files by running ci/scripts/build.sh locally, right? We don't need to add auto generated files for local test, right?

@kou
Copy link
Member

kou commented Aug 30, 2025

BTW, could you fix the lint failures?
https://github.com/apache/arrow-swift/actions/runs/17342772415/job/49252032542?pr=77

FYI: You can run CI on your fork https://github.com/morenonicolelli/arrow-swift/actions by enabling GitHub Actions on your fork.

@kou
Copy link
Member

kou commented Aug 31, 2025

Ah, you mean that you can't edit the current implementations/tests on your Xcode with the current structure, right?
If we want to work on #82 before this, people who can edit the current implementations/tests with the current structure need to work on #82 because we need to edit the current implementations/tests for #82, right?

@willtemperley
Copy link

Ah, you mean that you can't edit the current implementations/tests on your Xcode with the current structure, right? If we want to work on #82 before this, people who can edit the current implementations/tests with the current structure need to work on #82 because we need to edit the current implementations/tests for #82, right?

Yes, exactly. Before #77 it wasn't possible to see the source in Xcode when opening from root.

We needed the rectified structure in #77 to make the tests run, and we need the tests to pass to get #77 merged.

@kou
Copy link
Member

kou commented Aug 31, 2025

We don't need to add auto generated files:

0001-Don-t-add-auto-generated-files.patch
(You can apply it by git am < 0001-Don-t-add-auto-generated-files.patch.)

From 76bef0db149c659e2c53c1c1b5e86257b737fa2d Mon Sep 17 00:00:00 2001
From: Sutou Kouhei <[email protected]>
Date: Sun, 31 Aug 2025 12:38:20 +0900
Subject: [PATCH] Don't add auto generated files

---
 .../ArrowTests/Resources/testdata_bool.arrow  | Bin 650 -> 0 bytes
 .../Resources/testdata_double.arrow           | Bin 698 -> 0 bytes
 .../Resources/testdata_struct.arrow           | Bin 810 -> 0 bytes
 ci/scripts/build.sh                           |  25 +++++++++++++-----
 4 files changed, 19 insertions(+), 6 deletions(-)
 delete mode 100644 Tests/ArrowTests/Resources/testdata_bool.arrow
 delete mode 100644 Tests/ArrowTests/Resources/testdata_double.arrow
 delete mode 100644 Tests/ArrowTests/Resources/testdata_struct.arrow

diff --git a/Tests/ArrowTests/Resources/testdata_bool.arrow b/Tests/ArrowTests/Resources/testdata_bool.arrow
deleted file mode 100644
index 8dd4a94868da76305004f4513748cda452bfb462..0000000000000000000000000000000000000000
GIT binary patch
literal 0
HcmV?d00001

literal 650
zcmc&y%L&3j5PfRmkAPo9umKNV1dkqK1%ekVh<Gv(6v0X?!7@C0@L&P<qVMg_20L)V
z%bT5FW@fY6Y}VU3@S$7*bP1r1glHk++iaBxO+KK_zkw=GOJ@Pz%vY|@N1RZXJZZ0y
zkLR4eq4RgFLWm!_NYpKtU(f?6FF$0?tNtF;ESCL^s1L{y0i-Uf^w-q+n4Tv6nn8Jc
zBDOfuudeS&G@GR2Da>Q1az57fGBqBQ5q>YBcBm81A#7qwFBr(`p+TtM4xyTttjM_o
yuJ5=nvTT>13R?YriM6wY)Aemu?LxL0u^?1cJ=~lQyo^)hK>xbiI~!QJfA9<Osx~qJ

diff --git a/Tests/ArrowTests/Resources/testdata_double.arrow b/Tests/ArrowTests/Resources/testdata_double.arrow
deleted file mode 100644
index 011afd14f120d681c5c5887f6b1d4bbc97549d95..0000000000000000000000000000000000000000
GIT binary patch
literal 0
HcmV?d00001

literal 698
zcmd5)J5B>Z41Js3gb+pI7e#64DJYVTj!8#{v`x6cMyXh76@<i1NR=aSjFgmIBPW3O
zJhp^50Uo{acRaRdmUaEOS^(dqJ%Dop7-3ADqT=qhqQa01wB;S>QA=h4zIa||Cj_aV
z^C(&iD2eTB%ls3&<IJ-tPM8s-(mBNeBL-Zeo}F)>8>{m`i{HaKr<AYAD*|{@ILZB`
zI+)Qjq+h+rz9ITV&~G_x!Eam$UbsGY9`!!B9(_yIeE#MRJ@V|{MaTQhi?lWxrL12h
z<pHMYIt#XIp27*Co^|J2)3u!McGES@x_#;No-~_p%&sqe1`&Tt-<<-ONws{K4179|
QIl5}>KfJ%{TiT!f0k*(B?EnA(

diff --git a/Tests/ArrowTests/Resources/testdata_struct.arrow b/Tests/ArrowTests/Resources/testdata_struct.arrow
deleted file mode 100644
index b35387dc4f2cb68004e32083b46aff3a5b41e580..0000000000000000000000000000000000000000
GIT binary patch
literal 0
HcmV?d00001

literal 810
zcmdUuy-EW?6opUB8dq6SG)T&n78bFyG`%3CvDZQbHJ}l+_faf;5Fa6rVCfsg@66mQ
z*x7oPGk5Npx$|>&cRHJ$UmQy-u#~h5r7d-EQw?osy+$pM&}u97N+!Ki>$(vH+pjch
z@C;giWBioZz-7#>`)JUi86W5y#JiXA<$Qii-LTZ|#BQE%4k1{Nde-zK&RS|87Y{f0
z*T3?os|7jBuRe)U&;1VQ?-Ls+oj@bT`@Z>zk(P0LVEq~PVPcRSqR(L18^hihdCrXE
zqE>(0J&Zi-Cy{5pQ}3DkrvBGF=Xo91`n{MF-no(E)(rdP=<{~mdM`Y%N7jyD^{OfB
X*;C`Ih94gzE#W_ZH!Y@3%RllBNe@lz

diff --git a/ci/scripts/build.sh b/ci/scripts/build.sh
index 3343f9a..770b93f 100755
--- a/ci/scripts/build.sh
+++ b/ci/scripts/build.sh
@@ -43,13 +43,26 @@ if [ -d /cache ]; then
 fi
 github_actions_group_end
 
+github_actions_group_begin "Generate data"
+data_gen_dir="${build_dir}/source/data-generator/swift-datagen"
+if [ -d /cache ]; then
+  export GOCACHE="/cache/go-build"
+  export GOMODCACHE="/cache/go-mod"
+fi
+export GOPATH="${build_dir}"
+pushd "${data_gen_dir}"
+go get -d ./...
+go run .
+mkdir -p ../../Tests/ArrowTests/Resources/
+cp *.arrow ../../Tests/ArrowTests/Resources/
+popd
+github_actions_group_end
+
 github_actions_group_begin "Use -warnings-as-errors"
-for package in . Arrow ArrowFlight; do
-  pushd "${build_dir}/source/${package}"
-  sed 's/\/\/ build://g' Package.swift > Package.swift.build
-  mv Package.swift.build Package.swift
-  popd
-done
+pushd "${build_dir}/source"
+sed 's/\/\/ build://g' Package.swift > Package.swift.build
+mv Package.swift.build Package.swift
+popd
 github_actions_group_end
 
 github_actions_group_begin "Build"
-- 
2.50.1

@willtemperley
Copy link

Tests and lint are green on my fork. I'm seeing an intermittent error with GPG - it's failing on 5.10 now:

https://github.com/willtemperley/arrow-swift/actions/runs/17352604089/job/49261220123

However the previous commit with the auto-generated file patch applied failed on 5.10 and 6.1 with GPG errors:

https://github.com/willtemperley/arrow-swift/actions/runs/17352405747

@kou
Copy link
Member

kou commented Aug 31, 2025

It's a known problem: swift-actions/setup-swift#694

We need to ignore the flakiness for now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improving foldering structure
4 participants