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

ci: include config_proc_macro crate in ci #5389

Merged
merged 5 commits into from
Jun 21, 2022

Conversation

tommilligan
Copy link
Contributor

Found while working on #5379

@calebcartwright
Copy link
Member

In general I'm in favor of this but a few thoughts/questions:

  • Why the lockfile version change? Wasn't expecting to see a diff there
  • Does GitHub run shell-based scripts in a WSL or msys/cygwin type contexts on Windows? If so I'd be hesitant to make such a change, as I believe we should have Windows CI running in the most typical Windows context (cmd or powershell) to guard against any extreme edge cases

@tommilligan
Copy link
Contributor Author

tommilligan commented Jun 19, 2022

  • Why the lockfile version change? Wasn't expecting to see a diff there

It looks like the lockfile is actually out of date - see this diff in the version number of the config_proc_macro crate itself: 8184665#diff-863805b89377afaab3c52aeb1acb2f799c15f3e0789b35564866ed456c684df1R25

This indicates that at some point in the past (presumably for the v0.2.0 update), the update lockfile wasn't checked in.

To prevent this in future for lockfiles generally, we should run cargo build --locked in CI, which will fail if the lockfile is out of date. I've added this in another commit.

An orthogonal question to this is: do we actually want to commit a lockfile for the config_proc_macro crate? As it's a library crate, not a binary, this is actually in opposition to general cargo guidance: https://doc.rust-lang.org/cargo/faq.html#why-do-binaries-have-cargolock-in-version-control-but-not-libraries

  • Does GitHub run shell-based scripts in a WSL or msys/cygwin type contexts on Windows? If so I'd be hesitant to make such a change, as I believe we should have Windows CI running in the most typical Windows context (cmd or powershell) to guard against any extreme edge cases

Yes, you're right, it uses Git Bash which I believe is msys based. I've updated to use the native cmd shell, and added a translation of the ci bash script as a batch file. This was a fun not-so-fun throwback to writing batch files from many years ago, but I've run it through some failure cases on a CircleCI VM and am happy it accurately mirrors the linux CI process now.

Copy link
Contributor

@ytmimi ytmimi left a comment

Choose a reason for hiding this comment

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

Huge thanks for writing the batch script for windows, doing some research into how GitHub Actions runs the bash shell on windows, and for taking the time to test all this on CircleCI!!

An orthogonal question to this is: do we actually want to commit a lockfile for the config_proc_macro crate? As it's a library crate, not a binary, this is actually in opposition to general cargo guidance

Personally I'm not too sure how we want to handle this. I read through the linked article and it seems likely that we don't need to include the lockfile for the config_proc_macro crate.

That being said, I'm happy to move forward with the proposed changes and defer the lockfile discussion to a future PR

Copy link
Member

@calebcartwright calebcartwright left a comment

Choose a reason for hiding this comment

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

I'd have been fine with keeping the windows content inline in the Actions Workflow definition as it's not needed in other jobs and I'm not worried about changing CI engines any time soon.

However, let's give it a whirl since you spent the time and effort to get it working 😁 (I have some nightmares from Windows-scripting days too)

@calebcartwright calebcartwright merged commit 2c8b3be into rust-lang:master Jun 21, 2022
@calebcartwright
Copy link
Member

As for the proc macro lockfile, I'm not too concerned about it one way or the other. We've no intentions of publishing it as a standalone lib crate on crates, so the only consumers of interest are those in-repo.

It probably could still be dropped, but not a high priority and would also need to be considered in the context of building both in this repository and in r-l/rust builds

@tommilligan tommilligan deleted the ci-proc-macro-config branch June 21, 2022 15:57
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.

3 participants