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

Set up simple GitHub Actions workflow #3

Merged
merged 5 commits into from
Jan 4, 2020
Merged

Conversation

Xanewok
Copy link
Collaborator

@Xanewok Xanewok commented Dec 27, 2019

...so we can easily ensure compiling and passing build on Windows (and probably stuff like formatting if we feel like, later on). See https://github.com/Xanewok/win-crypto-ng/runs/364545278 for an example.

I used 1.34.0 since that's what's available in stable Debian and what's currently used to build Sequoia.

@emgre emgre force-pushed the gh-actions branch 2 times, most recently from f9b6e55 to 856e690 Compare December 27, 2019 18:07
@emgre
Copy link
Owner

emgre commented Dec 27, 2019

Nice, thank you for you PRs! I will merge them, starting by this one (so we have some CI going on before merging the others). Glad that this little pastime of mine actually helps someone.

I want the CI to run on stable and beta, as well as the MSRV. I'm open to support 1.34 if you use it in Sequoia, but apparently zeroize supports only Rust 1.39+. Were you able to compile it on 1.34? Because the GitHub Actions seems to fail (see https://github.com/emgre/win-crypto-ng/runs/365365652). Also, be aware that your original action file was actually running on stable, not 1.34.

@Xanewok
Copy link
Collaborator Author

Xanewok commented Dec 28, 2019

Woops, I thought I used the 1.34 in my Actions workflow, nice catch! I'm doing everyday development on most recent stable/nightly so I didn't catch that earlier.

If you don't mind, I also opened #4 to support lower MSRVs, so it might be a good idea to merge that first or merge this PR without 1.34 checks for now? Either way, thanks for taking a look!

Copy link
Collaborator Author

@Xanewok Xanewok left a comment

Choose a reason for hiding this comment

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

Thanks for adjusting my changes here!

I saw you merged a master before applying the change. What's is your preferred way of doing development here? I can rebase my changes on the most recent master if you want, to remove the noisy merge commits and simplify the history.

.github/workflows/rust.yml Outdated Show resolved Hide resolved
@emgre
Copy link
Owner

emgre commented Dec 29, 2019

That merge was not supposed to be pushed on this PR, I was just trying out stuff and I wanted to push in my repo, not yours. That env thing is also a test that I removed.

@emgre
Copy link
Owner

emgre commented Dec 29, 2019

I was able to make all the checks pass on stable and beta, but sadly, I use std::mem::MaybeUninit that was stabilized in Rust 1.36 and aliases in enum variants that was stabilized in Rust 1.37. Therefore, the lowest MSRV that I can do is 1.37...

@Xanewok
Copy link
Collaborator Author

Xanewok commented Dec 29, 2019

Yeah, I just pushed the adjustments to the branch here. Let's work with 1.37 first and see how far we can go from there for the time being 😅

@emgre
Copy link
Owner

emgre commented Dec 29, 2019

@emgre
Copy link
Owner

emgre commented Dec 29, 2019

I'm happy with the changes I did in my gh-actions branch. I suggest that I force push this in your repo and then merge in master. You'll still have commit 9d234d8 in the history, showcasing what you did.

@Xanewok
Copy link
Collaborator Author

Xanewok commented Dec 30, 2019

I'm happy with your changes to the workflows and I'm fine with you being the author here!

It's your repository and you'll do as you see fit, however I'd like to humbly propose that:

  1. we don't introduce new code via merge conflict resolution
  2. rebase the feature branches on top of master rather than back-merging it to feature branches

The first one allows to quickly see what introduced important changes to the code (commits) vs what simply adapts the code wrt branches/features merges (merge commits) and also simplifies common workflow operations like cherry-picking (we don't have to manually pick specific commit parent, we don't discard other parent's history etc.).

The second one reducing branching factor since the feature branch is always isolated and viewed in the context of a single master parent branch, rather than also including all the merge commits for all the intermediate states of the master parent branch.

To better illustrate my point, here are hypothetical merges of this PR:

Without rebasing
image

With rebasing
image

If you'd like to merge the rebased commits, I pushed them just now on my branch here.

As a side note, maybe we should enforce formatting outside of this PR? It might be a good idea to think whether we want to have a custom config here (e.g. I'm also a fan of use_small_heuristics which you seem to also be using)
EDIT: On a second thought, maybe it's better to have a default config enforced here.

@Xanewok
Copy link
Collaborator Author

Xanewok commented Jan 3, 2020

Hey 👋

Did you look at the recent changes here, by any chance?

@emgre
Copy link
Owner

emgre commented Jan 4, 2020

Sorry about the delay, I was quite busy. 😅 Thanks for your patience and dedication. Your rebasing makes much sense, I'm just not used to it, but I like the history it produces.

I had the opportunity to setup a CI for a Rust project at work, so I integrated what I learn in this project, namely:

  • I made a few changes to the config, renaming the workflow to "CI"
  • I added a security audit using RustSec everytime the Cargo.toml is changed and on every Monday morning.
  • I added doc-comment to compile and test the README.
  • I added a CI badge, an MSRV badge and a license badge to the README.

Now I'll (finally) merge.

@emgre emgre merged commit f9df604 into emgre:master Jan 4, 2020
@Xanewok Xanewok deleted the gh-actions branch January 4, 2020 19:51
@Xanewok
Copy link
Collaborator Author

Xanewok commented Jan 4, 2020

Thank you and no worries at all!

As a side note, I think it'd be good to only pull doc-comment via dev-dependencies, so that the dependent crates won't have to pull the unnecessary (for them) dependency.

@emgre
Copy link
Owner

emgre commented Jan 4, 2020

I tried that first, but it was not working inside a #[cfg(test)] block... Running cargo test was not running the README examples.

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