-
Notifications
You must be signed in to change notification settings - Fork 19
Conversation
fb50909
to
8201ec8
Compare
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.
Updating action for Windows signing makes sense, no concerns there.
.github/workflows/release.yml
Outdated
@@ -113,7 +113,13 @@ jobs: | |||
run: | | |||
echo "Importing certificate" | |||
echo "${{ secrets.MACOS_CERTIFICATE }}" | base64 --decode > certificate.p12 | |||
security create-keychain -p "${{ secrets.MACOS_CERTIFICATE_PASSWORD }}" build.keychain | |||
# Check if the keychain exists |
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.
This must not be necessary or else we might leak some keys from one workflow to another. There must be some state sharing here that causes an issue that is not present in GitHub's runners and thus needs to be fixed.
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 checked the runner and the keychain already exists on the machine, hence why the error "the keychain exists already". What do you propose to do to solve this? What about Apple-Actions/import-codesign-certs#8 (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.
I'm fairly certain it exists because of the command that was ran earlier. I don't know how runners work, but it is expected that every time we have a clean environment with no key chains and nothing in them. Maybe previous workflow run created it, but then something must ensure it doesn't exists anymore after it is over or else, as mentioned above, we're leaking keys.
And we need to check the same for Windows. It may not complain, but keys might still be left in there.
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.
The cleanup script should wipe this now after each run for macOS; for windows, it stores it in the path where jobs are ran and cleaned up, so should be fine.
8201ec8
to
51dc375
Compare
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.
Just triggered here: https://github.com/subspace/subspace-cli/actions/runs/5672898609
Let's see how it goes.
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.
Nope, release CI failed
- altool is deprecated and will be dropped in 2023 - altool also not available with xcode command-line tools
- use one protoc package as the bug was fixed with using powershell 7
a4990ab
to
06e442f
Compare
06e442f
to
fdb2994
Compare
macOS stapling not supported for .zip files
b0b4064
to
e18ca00
Compare
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.
Makes sense overall. Can you also replace github.github.repository_owner
with github.repository_owner
in workflows now (it was a typo and the reason why CI was passing before even if signing was failing).
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.
This is great, thanks!
Was CI not triggered here because only workflow file has changed?
it was triggered but I just cancelled the running, will work on converting it to using homebrew for installation. |
I mean I don't see testing/lining in this PR in list of checks. If they were cancelled, it'd show something that failed/was cancelled, but it shows one expected and nothing else. |
- use brew instead of downloading releases
This PR:
v2.0.1
to support x64 arch and newer windows sdk build versions.