-
-
Notifications
You must be signed in to change notification settings - Fork 4.9k
build: Add GitHub actions build for Windows #8627
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
This is sort of a proof of concept, but since our current Windows builder is down this might solve the problem. It includes a change for easier code signing (taking the certificate in a secret/env var rather than existing already on disk), but otherwise mirrors precisely what we already do in the build server.
imsodin
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.
Works on the PR (GH is smart enough to pick up pipeline changes for PR runs - nice?!) and looks as tidy as yaml can.
One minor downside is that artifacts from GH actions are only accessible to logged in GH users. I don't think there's much need for those for non-github users though - otherwise we could setup an action to upload it somewhere (nightly section on the downloads page or something).
| # Only run this for pushes to our branches, not on PRs. Uses secrets | ||
| # for code signing. | ||
| if: github.event_name == 'push' | ||
| run: | |
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 feels a bit weak.
I know that only we can push to "our" branches, but I'd felt more comfortable of this had some sort of branch name check etc, or something.
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.
Right - I'd even say we don't need any CI for random branches. So a branches: ["main"] condition at the top on the triggers could ensure that.
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.
But we still want to build on PRs to check that a given PR doesn't break the Windows build, don't we?
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 are two triggers: One for PR and one for push, which are independent. The branch restrictions is only needed on the push one.
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... I was thinking about that. We currently produce artifacts for any all builds on random branches, and that might be something we want to preserve? Otherwise we can of course easily restrict it to just builds on main.
(I also have a much more elaborate extension of this PR, with matrix builds of go version x platform and package builds for all the things we usually build for...)
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.
Would it be possible to exfiltrate e.g signing certificate that way?
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 so; PRs don't get access to secrets, so we'd have to approve and merge the exfiltrating code first. Probably you can do that with our current build setup, though... If you are a contributor who can push directly to the repo, then yes you could exfiltrate the secrets. Again, you can do that today also because they are environment variables or files on disk in the current setup.
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... I was thinking about that. We currently produce artifacts for any all builds on random branches, and that might be something we want to preserve? Otherwise we can of course easily restrict it to just builds on main.
(I also have a much more elaborate extension of this PR, with matrix builds of go version x platform and package builds for all the things we usually build for...)
do random branch builds need to be signed?
don't think so.
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.
No, they really don't. GitHub actions has an "environments" concept where you can bind secrets to an environment and say that a certain branch should run in that environment, I think. I'll see if I can make the signing secrets specific to builds on main and release branches only.
* main: (36 commits) lib/protocol: Ignore inode time when xattr&ownership is ignored (fixes syncthing#8654) (syncthing#8655) lib/fs: Try to remove read only Windows files (fixes syncthing#3744) (syncthing#8650) gui: Add copy to clipboard, share by email, and share by SMS buttons to device IDs (fixes syncthing#2771, ref syncthing#3868) (syncthing#7984) gui, man, authors: Update docs, translations, and contributors build: Add GitHub actions build for Windows (syncthing#8627) gui: Fix connection type icon width (fixes syncthing#8592) (syncthing#8644) gui: Adjust connection type icon size scaling and alignment (syncthing#8645) docker: Use healthcheck endpoint (syncthing#8640) lib/connections: Use adaptive write size for rate limited connections (fixes syncthing#8630) (syncthing#8631) gui: Mark devices that haven't connected for a long time (fixes syncthing#7703) (syncthing#8530) gui: Fix rescan interval when add encrypted folder with watch for changes enabled (fixes syncthing#8570) (syncthing#8571) gui: Always show Out of Sync Items for remote devices (syncthing#8632) lib/fs: Let xattr test avoid non-test attributes (fixes syncthing#8601) (syncthing#8628) build: Add GitHub actions build for Windows gui, man, authors: Update docs, translations, and contributors gui: Display folder and device count number (syncthing#8615) gui, man, authors: Update docs, translations, and contributors lib/model, lib/protocol: Fix file comparisons (fixes syncthing#8594) (syncthing#8603) lib/scanner: More sensible debug output (syncthing#8596) gui: Allow automatic device ID selection on WebKit browsers (ref syncthing#8544) (syncthing#8597) ...
* main: (23 commits) lib/fs: Optimize WindowsInvalidFilename (syncthing#8687) gui, man, authors: Update docs, translations, and contributors cmd/syncthing: Use main logger in generate subcommand (fixes syncthing#8682) (syncthing#8685) build: Update all dependencies (fixes syncthing#8679) (syncthing#8680) gui, man, authors: Update docs, translations, and contributors lib/model: Correctly set xattrs on temp files (fixes syncthing#8667) (syncthing#8670) gui: Automatically dismiss authentication reminder when in LDAP mode (fixes syncthing#8661) (syncthing#8663) lib/model: Correctly handle xattrs on directories (fixes syncthing#8657) (syncthing#8658) lib/protocol: Ignore inode time when xattr&ownership is ignored (fixes syncthing#8654) (syncthing#8655) lib/fs: Try to remove read only Windows files (fixes syncthing#3744) (syncthing#8650) gui: Add copy to clipboard, share by email, and share by SMS buttons to device IDs (fixes syncthing#2771, ref syncthing#3868) (syncthing#7984) gui, man, authors: Update docs, translations, and contributors build: Add GitHub actions build for Windows (syncthing#8627) gui: Fix connection type icon width (fixes syncthing#8592) (syncthing#8644) gui: Adjust connection type icon size scaling and alignment (syncthing#8645) docker: Use healthcheck endpoint (syncthing#8640) lib/connections: Use adaptive write size for rate limited connections (fixes syncthing#8630) (syncthing#8631) gui: Mark devices that haven't connected for a long time (fixes syncthing#7703) (syncthing#8530) gui: Fix rescan interval when add encrypted folder with watch for changes enabled (fixes syncthing#8570) (syncthing#8571) gui: Always show Out of Sync Items for remote devices (syncthing#8632) ...
This is sort of a proof of concept, but since our current Windows builder is down this might solve the problem. It includes a change for easier code signing (taking the certificate in a secret/env var rather than existing already on disk) but otherwise mirrors precisely what we already do in the build server.