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

Add Windows CNGCrypto AMD64 to dist and release assets. #824

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

mattdurham
Copy link
Collaborator

PR Description

This adds windows executable for cngcrypto amd64 support.

PR Checklist

  • CHANGELOG.md updated
  • Documentation added
  • Tests updated via drone step

@mattdurham mattdurham requested a review from rfratto May 10, 2024 16:08
@mattdurham mattdurham marked this pull request as ready for review May 10, 2024 16:08
@rfratto
Copy link
Member

rfratto commented May 10, 2024

Thanks for working on this. I think we're getting to the point where too many details of our CI are leaking into our build system, which goes against the intended spirit of the current Makefile:

  • The Makefile should only have targets useful to both local development and CI
  • We should consistently use the same build image for CI

If we absolutely need this PR to be merged right this moment, I think we can do that. But there's a few signals I'm seeing right now (no particular order):

  • The need to produce CNGCrypto assets which depend on different build images (this PR)
  • The growing complexity of the Makefile (as you mention in Proposal: Change our makefile process #826)
  • @erikbaranowski's idea to switch from NSIS to Nix (which needs Windows to build)
  • Our issues with Drone no longer allowing us to use Docker-in-Docker in PR pipelines

These signals indicate that it's about the right time for redrawing the lines between what is needed for day-to-day local development, and what is needed primarily for our CI pipelines.

So if CNGCrypto assets can wait a bit longer in the 1.2 cycle, I think it will be valuable to see if we can align all of these issues under a single plan.

@erikbaranowski
Copy link
Contributor

@rfratto I agree with needing a plan on how to improve the sanity of our CI and builds. I had hoped to tackle a piece of this with the windows MSI work, but we probably need to lay out a logical plan of what things can be improved in order to get us where we want to end up.

@mattdurham
Copy link
Collaborator Author

Its totally fine to wait, I wanted it to be ready so it def makes 1.2.

@mattdurham
Copy link
Collaborator Author

More universally we could switch to almost one build image but it would be the Microsoft one and that concerns me a bit. It might be an unneeded concern since they are very prompt on updated it due to CVEs or releases.

@rfratto rfratto added the backport-to-agent:no PR should NOT be backported to the agent repo. label May 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-to-agent:no PR should NOT be backported to the agent repo.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants