-
Notifications
You must be signed in to change notification settings - Fork 105
GH-6: Add Linux test CI #339
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Fixes #6.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about test.yml instead of java.yml?
.env
Outdated
| # coredump generation set it to 0 | ||
| ULIMIT_CORE=-1 | ||
|
|
||
| # Default versions for platforms |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we remove this?
.env
Outdated
| ARCH_SHORT=amd64 | ||
|
|
||
| # Default repository to pull and push images from | ||
| REPO=apache/arrow-dev |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about using ghcr.io/apache/arrow-java-dev?
If we use Docker Hub, we need to set Docker Hub secrets.
.github/workflows/java.yml
Outdated
| uses: actions/cache@13aacd865c20de90d75de3b17ebe84f7a17d57d2 # v4.0.0 | ||
| with: | ||
| path: .docker | ||
| key: maven-${{ hashFiles('java/**') }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you update the hashFiles() argument?
| restore-keys: maven- | ||
| - name: Execute Docker Build | ||
| env: | ||
| DEVELOCITY_ACCESS_KEY: ${{ secrets.GE_ACCESS_TOKEN }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to set this secret?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, we'll need to ask Infra to set it up again - this implements build caching (I'll have to figure out the details). It's not strictly required, though
arrow-format/README.rst
Outdated
| This folder contains binary protocol definitions for the Arrow columnar format | ||
| and other parts of the project, like the Flight RPC framework. | ||
|
|
||
| For documentation about the Arrow format, see the `docs/source/format` | ||
| directory. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about mentioning that this is a copy of https://github.com/apache/arrow/tree/main/format ?
We may want to mention the revision of the copy.
Co-authored-by: Sutou Kouhei <kou@cozmixng.org>
kou
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
| docker compose run \ | ||
| -e CI=true \ | ||
| -e "DEVELOCITY_ACCESS_KEY=$DEVELOCITY_ACCESS_KEY" \ | ||
| ${{ matrix.image }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need docker compose pull/docker compose push to cache built images but let's work on it as a separated task.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry. We don't need this. Because we use existing Docker images...
We can remove REPO from .env.
| uses: actions/cache@6849a6489940f00c2f30c0fb92c6274307ccb58a # v4.1.2 | ||
| with: | ||
| path: .docker | ||
| key: maven-${{ matrix.jdk }}-${{ matrix.maven }}-${{ hashFiles('**/docker-compose.yml') }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We may want to update cache by .java changes. Let's work on it as a separated task.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, since the docker image is just jdk/maven, shouldn't it not depend on the actual Java sources?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that this is used to cache mvn ... results. So the cache will depend on *.java too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, the build script starts by wiping the build directory, though. (And we have separate build caching support.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I'm not familiar with Maven cache...
This .docker path caches ~/.m2/ directory. Do we need to cache ~/.m2/? If we don't need to cache ~/.m2/, we can remove this cache.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, this is confusing then. If it caches .m2 then it'll cache downloaded dependencies, which is useful, and that depends on pom.xml (not .java though)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll update this to hash pom.xml in the new PR
Fixes #6.