-
Notifications
You must be signed in to change notification settings - Fork 172
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
Add GitHub actions #279
Add GitHub actions #279
Conversation
Re the third commit, I recall with OpenSSL 1.1.0 or 1.1.1 (vs 1.0.2) the following issues with CI:
Tested locally using OpenSSL 1.1.1d and |
- name: repo checkout | ||
uses: actions/checkout@v1 | ||
with: | ||
fetch-depth: 10 |
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 10 and not 1 as in the example? We're not going to be doing anything with the history?
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.
Because several CI builds could be queued up at the same time.
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.
According to https://github.com/actions/checkout and https://help.github.com/en/github/automating-your-workflow-with-github-actions/events-that-trigger-workflows every invocation of the action checks out a specific SHA, which includes any previous changes up to that point. I don't see how those other 9 commits will be used no matter any queueing going on?
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.
https://github.com/actions/checkout/blob/master/action.yml#L17-L18
Specifically, 'defaults to no limit'
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, earlier I was distracted. Without fetch-depth, all commit history is retrieved via git fetch. So, it should be larger than the maximum number of commits that one expects to be in the CI 'pipeline' at an given time.
Without it, large repos can take longer to retrieve...
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'm not arguing against doing a shallow clone, but against using a depth of 10 instead of 1 🙂 Every queued build will run the action and get their own history up to the SHA checked out. There's no need for every build fetching 9 more commits of history. They won't be used, there's no sharing going on.
Also see actions/checkout#22 where further optimizations are discussed.
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 ruby/ruby did their own command, similar to what's shown, as they have a lot of tags & commits.
Regardless, the longest checkout step when run in my fork took 5 seconds. We've spent more time here than the next several commits/pr's would take...
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.
@bdewater please submit an updated PR after this merged and we can confirm your thoughts, thanks everyone for the discussion and work.
Ubuntu 16.04 - Ruby 2.3, OpenSSL 1.1.1
Ubuntu 18.04 - Ruby 2.4, 2.5, 2.6, OpenSSL 1.1.1
macOS - 2.4, 2.5, 2.6, OpenSSL 1.0.2
Windows - Ruby 2.4, 2.5, 2.6, OpenSSL 1.0.2 & 1.1.1
Commits:
'Add GitHub Actions- Ubuntu, macOS, Windows' - adds Actions, MinGW jobs fully update MSYS2 toolchain and add correct OpenSSL files.
'extconf.rb - update for new MSYS2, libssp' - current MSYS2 tools require using libssp.
'OpenSSL::TestSSL#test_finished_messages - gracefully close client' - test would not pass on MinGW build using 1.1.1d. Added two lines to close client