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

update to 1.64 and use workspace dependencies #445

Closed
wants to merge 0 commits into from

Conversation

justinmmott
Copy link

currently trybuild does not work properly with workspace defined dependencies. So, serde and proc-macro2 have been left alone in the dropshot/Cargo.toml file. I have opened an issue on the trybuild repo to try and resolve this.

@justinmmott
Copy link
Author

currently trybuild does not work properly with workspace defined dependencies. So, serde and proc-macro2 have been left alone in the dropshot/Cargo.toml file. I have opened an issue on the trybuild repo to try and resolve this.

No longer an issue as trybuild was updated

@justinmmott justinmmott changed the title update to 1.64 and start impl of using workspace dependencies update to 1.64 and use workspace dependencies Sep 26, 2022
@justinmmott
Copy link
Author

dependabot/dependabot-core#5315 seems that this wouldn't fix this issue as some of the test errors have changed slightly

@davepacheco
Copy link
Collaborator

@justinmmott Thanks for doing this. I see Renovate also opened a PR to update to 1.64 in #444. We can close that one if we wind up moving forward with this one.

What does using workspace dependencies mean for consumers? Does it mean that consumers must be using 1.64 or newer?

@justinmmott
Copy link
Author

@davepacheco Sounds good to me. I'm not sure about what the workspace dependencies will do for customers, I was hoping the change to the rust-toolchain.toml would alleviate any issues with that. Also, #typify/113 has some discussion on the matter as well. However, in this case I don't think that dependabot would be able to make the changes in the .stderr files.

@davepacheco
Copy link
Collaborator

I believe rust-toolchain.toml is used only when people clone and build dropshot directly. But if you've got an application (like our own omicron) that uses dropshot, then dropshot's rust-toolchain.toml is not used. We use rust-toolchain in dropshot mainly to communicate (and enforce) that dropshot devs are using a consistent toolchain. Dropshot itself is expected to work with a wider range of Rust versions (though we've never really said what that is, either). I think we'd at least want to make sure our own projects won't break with this change.

@davepacheco
Copy link
Collaborator

Ah I see @ahl also called out the dependency on dependabot/dependabot-core#5315 -- that'd apply here too.

@davepacheco
Copy link
Collaborator

FYI I went ahead and updated and landed #444 since it seems like there are still some open questions here.

@justinmmott
Copy link
Author

justinmmott commented Sep 27, 2022

Accidentally discard my changes due to trying to merge main into my fork. Didn't realize the ui would even let me do that so didn't look to closely lol. But it's fine because this can be closed as the depandabot change would be able to correctly change this now.

@davepacheco
Copy link
Collaborator

Ah okay. For what it's worth I think workspace inheritance is still promising here. I think we just want to make sure it won't break consumers (either by verifying that it doesn't, or by updating the ones we know about and bumping the major rev of this crate).

@justinmmott
Copy link
Author

Ah okay. For what it's worth I think workspace inheritance is still promising here. I think we just want to make sure it won't break consumers (either by verifying that it doesn't, or by updating the ones we know about and bumping the major rev of this crate).

Oh I totally agree, but if you guys are waiting on dependabot to make the updates automatically anyways. I can try and test to see if it will work down the line.

@poliorcetics
Copy link

I opened a PR for it dependabot/dependabot-core#5794, it should work but it's not merged yet.

It's probably possible to regenerate the docker container and use it locally to make the necessary changes but I have no idea how to do that. I'll gladly accept people testing it and telling me if it works has expected in a variety of configs though 👍

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