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

Upgrade to arbitrary 1.3 #271

Closed
wants to merge 1 commit into from

Conversation

fornwall
Copy link

@daxpedda
Copy link
Contributor

To summarize: currently arbitrary v1.0.0 doesn't actually compile with the minimum version of syn v1.0.0 they specify themselves. This was (probably unintentionally) fixed in arbitrary v1.3.

This was caught in the -Zminimal-versions check on CI by naga.

@cuviper
Copy link
Member

cuviper commented Aug 10, 2023

Why wouldn't you just bump to arbitrary 1.3 in naga's dependency?

I also don't understand why this is a new problem for you with indexmap 2...

@daxpedda
Copy link
Contributor

Why wouldn't you just bump to arbitrary 1.3 in naga's dependency?

True, I didn't consider this.

I also don't understand why this is a new problem for you with indexmap 2...

I just dug into this, the way -Zminimal-versions was used in nagas CI before was broken (I will provide a fix soon).


I guess indexmap doesn't need this PR anymore. It would probably be still good for indexmap to actually support building the minimal version it specifies (I'm happy to add a -Zminimal-versions run in the CI as well), if you don't deem it so, feel free to close this.

@cuviper
Copy link
Member

cuviper commented Aug 10, 2023

I'm on board with making sure that we have correctly specified our direct dependencies, but I'm not fond of checking -Zminimal-versions because of these problems with indirect dependencies. Indexmap does work fine with the API of arbitrary 1.0.0, but that full dependency chain needing syn 1.0.6 is not really our problem.

@cuviper
Copy link
Member

cuviper commented Aug 10, 2023

Ooh, I hadn't noticed -Z direct-minimal-versions before -- that would be more palatable.

@daxpedda
Copy link
Contributor

I'm on board with making sure that we have correctly specified our direct dependencies, but I'm not fond of checking -Zminimal-versions because of these problems with indirect dependencies. Indexmap does work fine with the API of arbitrary 1.0.0, but that full dependency chain needing syn 1.0.6 is not really our problem.

I'm unsure though what arbitrary should do at this point though, yank all their old versions? arbitary v1.3 actually works correctly.
I don't disagree though, it's a problem.

I'm gonna make a new PR then.

@cuviper
Copy link
Member

cuviper commented Aug 11, 2023

I'm unsure though what arbitrary should do at this point though, yank all their old versions? arbitary v1.3 actually works correctly.

Nah, it doesn't need to be yanked. It's a bug, but not a showstopper, only affecting users that want -Zminimal-versions to work. Such users can update to their requirement to the fixed version 1.3.

And I understand that there's not also an indexmap version that you could update to solve this from afar, but anyone using indexmap/arbitrary should also have their own arbitrary dependency that can require newer.

Closing in favor of #272, thanks!

@cuviper cuviper closed this Aug 11, 2023
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