-
Notifications
You must be signed in to change notification settings - Fork 17
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
More XML formatting and GH Actions fix #61
Conversation
repository: wala/IDE | ||
# fetch-depth: 50 | ||
path: ./IDE | ||
run: git clone --depth=50 https://github.com/wala/IDE ${{ runner.temp }}/IDE |
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.
What is the purpose of these changes to use an explicit git clone
? I don't quite get what is going on here.
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 for the lack of context. The XML format checker is going into the Jython3 code that is being cloned inside the project directory and causing a failure. The idea then is to move the cloned external dependencies outside the project directory, as was done in the original Travis CI configuration. However, the GitHub Actions only allows cloning in the project directory. Thus, we reverted to the explicit clone, as was also done in the original Travis CI configuration.
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.
Relevant lines from Travis CI config:
Lines 6 to 7 in 6c10952
- git clone --depth=50 https://github.com/wala/IDE /tmp/IDE | |
- git clone https://github.com/juliandolby/jython3.git /tmp/jython3 |
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.
Does it not work to keep using the checkout action but pass path: ${{ runner.temp }}/IDE
instead of path: ./IDE
?
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.
It does not, unfortunately. That variable returns a path outside the project directory but in the working space.
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.
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.
Ok, yes, this is indeed a limitation, sigh actions/checkout#197
repository: wala/IDE | ||
# fetch-depth: 50 | ||
path: ./IDE | ||
run: git clone --depth=50 https://github.com/wala/IDE ${{ runner.temp }}/IDE |
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.
Ok, yes, this is indeed a limitation, sigh actions/checkout#197
repository: wala/IDE | ||
# fetch-depth: 50 | ||
path: ./IDE | ||
run: git clone --depth=50 https://github.com/wala/IDE ${{ runner.temp }}/IDE |
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.
Why not --depth=1
?
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 don't see why not. This was also in the original Travis CI config. Travis CI clones a history of 50 the get the project sources. But, I found this post. Looks like we should stick with 50?
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 don't think the issue on that post is a concern for PR CI jobs. If it comes up we can switch to 50, but for now I say depth 1.
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 not use a depth=1 on your project for CI!
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.
Well, I guess it was 1
before! https://github.com/actions/checkout#:~:text=%23%20Default%3A%201-,fetch%2Ddepth%3A%20%27%27,-%23%20Whether%20to
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.
Looks like there could be a problem with tags: actions/checkout#217 (comment).
We can try 1
. I don't foresee having a bunch of queued up builds.
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 also really like using the concurrency
setting for GitHub Actions:
This ensures there is only one build in progress for any PR or on the master branch. If you push a new commit to the PR branch, it cancels the ongoing build before starting a new one. If we want additional safety we can go with depth=1
and also add this concurrency
setting.
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.
Yeah, we have this kind of thing on Travis CI (there, it's specified on the web and not the YAML). The problem we've had with it is for builds emanating from tags. If you have a deploy action that only works on tagged commits, then you bump the version to the SNAPSHOT
version, the CI stops building the tagged version (that was supposed to be deployed) and starts building the new SNAPSHOT
version. The result is that the deployment gets skipped. Otherwise, I also like the feature :).
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.
If we ever need to do some kind of continuous deployment on GitHub Actions from tags, I'm sure we can configure it with a completely different .yml
file, and the concurrency
setting from one file shouldn't affect the other one.
with: | ||
repository: juliandolby/jython3 | ||
path: ./jython3 | ||
run: git clone https://github.com/juliandolby/jython3.git ${{ runner.temp }}/jython3 |
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 could do --depth=1
here 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.
See #61 (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.
See #61 (comment) :-)
git clone
directly to avoid XML formatting error in external dependencies by cloning into a temporary directory outside the project directory.