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

Requirement to precisely pin the version of cargo-expand #90

Closed
ijackson opened this issue Mar 26, 2023 · 8 comments · Fixed by #110
Closed

Requirement to precisely pin the version of cargo-expand #90

ijackson opened this issue Mar 26, 2023 · 8 comments · Fixed by #110

Comments

@ijackson
Copy link
Contributor

macrotest depends fundamentally on the stability of the output of cargo expand. But this is not something that cargo expand's maintainers promise: dtolnay/cargo-expand#179.

If one is using macrotest in CI, as one should, the CI will occasionally need to rebuild cargo expand. If it gets a new version, the tests break.

The solution is to use something like

cargo install --locked --version 1.0.44 --features=prettyplease cargo-expand

I think this should be documented.

@taiki-e
Copy link
Collaborator

taiki-e commented Mar 30, 2024

AFAIK, to make the output from macrotest completely stable, some things need to be pinned, not only cargo-expand.

  • macrotest (can be forced by = requirement in Cargo.toml)
  • macrotest's prettyplease dependency (can be forced by = requirement in Cargo.toml)
  • toolchain to run test (version of nightly toolchain used to be important, but is not required for the latest cargo-expand.)
  • cargo-expand (can be forced by --version <version> on installation)
  • cargo-expand's prettyplease dependency (can be forced by --locked on installation)

The most likely to affect the output would be prettyplease versions. If I understand correctly, what you actually encountered was due to the last one (probably the change in prettyplease 0.2.4).

And in the first place, it is odd that the version of cargo-expand's prettyplease dependency is also important, even though macrotest calls it. In other words, if we had passed the --ugly flag to cargo-expand to suppress formatting, we could probably get enough stable output by only pinning the macrotest/prettyplease version in the Cargo.toml.

@taiki-e
Copy link
Collaborator

taiki-e commented Mar 30, 2024

And in the first place, it is odd that the version of cargo-expand's prettyplease dependency is also important, even though macrotest calls it. In other words, if we had passed the --ugly flag to cargo-expand to suppress formatting, we could probably get enough stable output by only pinning the macrotest/prettyplease version in the Cargo.toml.

Hmm. I tried to do this (master...taiki-e:macrotest:ugly), but cargo-expand applies some additional changes to the non---ugly output, so doing it is a bit complicated.

@ijackson
Copy link
Contributor Author

And, yes, it's necessary to pin a lot of things. I'm not sure that macrotest's docs is the right place to mention them all so in #110 I mention only cargo expand since an actuall rune is provided in the macrotest docs.

If you like I can expand on the text in #110.

@ijackson
Copy link
Contributor Author

* macrotest (can be forced by `=` requirement in Cargo.toml)

CI tests need to use pinned Cargo.lock. (I think this is generally true, and therefore doesn't have to be discussed in macrotest's docs.)

@taiki-e
Copy link
Collaborator

taiki-e commented Mar 30, 2024

Assuming Cargo.lock is committed to the repository, I think the current #110 is enough.

@ijackson
Copy link
Contributor Author

Assuming Cargo.lock is committed to the repository, I think the current #110 is enough.

And the toolchain needs to be pinned. I'm not sure if we need to mention that.

Those two things are what I'm doing in derive-deftly and it is, empirically, reliable. (And, empirically, updating toolchain, edition, pinned cargo-expand, and so on, does cause test output changes.)

@taiki-e
Copy link
Collaborator

taiki-e commented Mar 30, 2024

And, empirically, updating toolchain, edition, pinned cargo-expand, and so on, does cause test output changes

As for the editions, there was a problem due to the lack of support for the newer ones, and I guess that is what you were encountering (#82). And that has been fixed in today's release in such a way that similar problems will not occur in the future (hopefully).

@ijackson
Copy link
Contributor Author

As for the editions, there was a problem due to the lack of support for the newer ones, and I guess that is what you were encountering (#82). And that has been fixed in today's release in such a way that similar problems will not occur in the future (hopefully).

Yes, I did need to pick that up indeed (and yes, thanks for #82 which lgtm), but there were other changes too. Anyway, I'm on 2021 now, successfully, so thanks :-).

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 a pull request may close this issue.

2 participants