Skip to content
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

Merged
merged 16 commits into from
Jun 23, 2023
Merged

Conversation

khatchad
Copy link
Collaborator

@khatchad khatchad commented Jun 22, 2023

  1. XML formatting was not recursing into subdirectories. Fixed.
  2. GitHub Actions CI config was not maintaining the correct working directories. Fixed.
  3. Use git clone directly to avoid XML formatting error in external dependencies by cloning into a temporary directory outside the project directory.
  4. Also recurse into subdirectories for other metadata files.

@khatchad khatchad changed the title More xml formatting and GH Actions fix More XML formatting and GH Actions fix Jun 22, 2023
@khatchad khatchad marked this pull request as ready for review June 22, 2023 18:55
@khatchad khatchad enabled auto-merge (squash) June 22, 2023 18:55
@khatchad khatchad requested a review from msridhar June 22, 2023 18:55
repository: wala/IDE
# fetch-depth: 50
path: ./IDE
run: git clone --depth=50 https://github.com/wala/IDE ${{ runner.temp }}/IDE
Copy link
Member

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.

Copy link
Collaborator Author

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.

Copy link
Collaborator Author

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:

ML/.travis.yml

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

Copy link
Member

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?

Copy link
Collaborator Author

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Copy link
Member

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
Copy link
Member

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
Copy link
Member

Choose a reason for hiding this comment

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

Why not --depth=1?

Copy link
Collaborator Author

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?

Copy link
Member

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Copy link
Collaborator Author

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.

Copy link
Member

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:

https://github.com/wala/WALA/blob/89de874efbc3369097e03b85493f4a3beb8e1a44/.github/workflows/continuous-integration.yml#L7-L9

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.

Copy link
Collaborator Author

@khatchad khatchad Jun 23, 2023

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 :).

Copy link
Member

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
Copy link
Member

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

See #61 (comment) :-)

@khatchad khatchad merged commit 6c8e096 into master Jun 23, 2023
@msridhar msridhar deleted the more_xml_formatting branch June 23, 2023 18:57
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.

2 participants