Loosen proc-macro2 version selection #1351
Conversation
Signed-off-by: salaheldinsoliman <salaheldin_sameh@aucegypt.edu>
|
Sorry, I think I made a mistake in the previous PR where I commented about the lock file. We need to keep Cargo.lock, and not have it in the gitignore. While there is a historical recommendation to not commit lock files in Rust libraries, we don't follow it because it is safer for developers working on the library if a lock file is present, otherwise fresh clones of the repo download new versions of crates outside the control of the developer, and for good security, developers should always be in control of what is installed/built/run on their systems. Could we keep the Cargo.lock file? It shouldn't need modifying as part of this change. |
| @@ -15,7 +15,7 @@ proc-macro = true | |||
| [dependencies] | |||
| syn = {version="=2.0.39",features=["full"]} | |||
| quote = "=1.0.33" | |||
| proc-macro2 = "=1.0.69" | |||
| proc-macro2 = "1.0.69" | |||
There was a problem hiding this comment.
To be clear, this is equivalent as saying >=1.0.69, <2.0.0.
There was a problem hiding this comment.
@graydon Wdyt? I think it's pretty tough for these libs to be pinning these deps, which makes it very incompatible with something requiring a newer version by happenstance via another dependency.
Very likely we should make the same changes to syn, quote, and itertools too.
Not merging this until we have more consensus.
There was a problem hiding this comment.
@graydon Wdyt? I think it's pretty tough for these libs to be pinning these deps, which makes it very incompatible with something requiring a newer version by happenstance via another dependency.
Very likely we should make the same changes to syn, quote, and itertools too.
Not merging this until we have more consensus.
|
Related discussion in Discord: https://discord.com/channels/897514728459468821/1067534037310255225/1230929191751647314 |
|
Related internal discussion: https://stellarfoundation.slack.com/archives/C030Z9EHVQE/p1713745643062029 |
@leighmcculloch Is there a way to have a peek on this discussion, as I don't have access to it? |
|
I think we're going to do this (in preference to #1415) but before we do, we'll write a tool that checks that the lockfile on the stellar-core repo is the same (at least on any package in common) with the lockfile in this repo. That ought to address the concern that the two might be testing / releasing different versions. I'll update here when I have such a thing ready. |
|
@salaheldinsoliman Since we're going to relax the version requirements rather than pin, did you want to also relax the version requirement of quote in this PR? |
|
here's an initial sketch of such a tool: https://github.com/graydon/check-lockfile-intersection |
Signed-off-by: salaheldinsoliman <salaheldin_sameh@aucegypt.edu>
|
👋🏻 What's the status of the |
|
I'm opening the following change instead that will unpin most of the deps and supersedes this change: |
### What Unpin all non-stellar dependencies. ### Why We have had reports that the libs are difficult to integrate into other projects due to the rigid dependency requirements. We ultimately need the dependencies to be pinned in stellar-core. There's benefit to the tests in this repo running with the same dependencies as what core pins to. There's benefits to stellar-rpc also using the same dependencies so as to decrease the chance that simulation runs with different behavior. Also, the same applies to the SDK for test behavior. Pinning the deps were an easy way to keep the dependencies consistent everywhere, but it also makes it difficult for folks to use the env libraries. Close #1351 ### Other things that need to happen @graydon is making a tool to check that Cargo.lock files are using consistent versions where possible across the env repo, core repo, and we can also use that in the rpc repo. It won't be practical to expect contract devs to use it on their contract projects, so contract devs will be able to use whatever combination of dependencies and that's just a limitation. ### Why not We could say again that not doing this has greater benefits than allowing folks to more easily use the libraries in their own projects.
### What Unpin all non-stellar dependencies. ### Why We have had reports that the libs are difficult to integrate into other projects due to the rigid dependency requirements. We ultimately need the dependencies to be pinned in stellar-core. There's benefit to the tests in this repo running with the same dependencies as what core pins to. There's benefits to stellar-rpc also using the same dependencies so as to decrease the chance that simulation runs with different behavior. Also, the same applies to the SDK for test behavior. Pinning the deps were an easy way to keep the dependencies consistent everywhere, but it also makes it difficult for folks to use the env libraries. Close #1351 ### Other things that need to happen @graydon is making a tool to check that Cargo.lock files are using consistent versions where possible across the env repo, core repo, and we can also use that in the rpc repo. It won't be practical to expect contract devs to use it on their contract projects, so contract devs will be able to use whatever combination of dependencies and that's just a limitation. ### Why not We could say again that not doing this has greater benefits than allowing folks to more easily use the libraries in their own projects. (cherry picked from commit a54d997)
What
1-Bump and loosen crates proc-macro2 and quote
Why
To solve a dependancy error while building Solang, when adding soroban-env-host to its Cargo.toml:
error: failed to select a version for
proc-macro2.... required by package
soroban-builtin-sdk-macros v20.2.2... which satisfies dependency
soroban-builtin-sdk-macros = "=20.2.2"of packagesoroban-env-host v20.2.2... which satisfies dependency
soroban-env-host = "^20.2.2"of packagesolang v0.3.3versions that meet the requirements
=1.0.69are: 1.0.69all possible versions conflict with previously selected packages.
previously selected package
proc-macro2 v1.0.78... which satisfies dependency
proc-macro2 = "^1.0.78"of packageparse-display-derive v0.9.0... which satisfies dependency
parse-display-derive = "=0.9.0"of packageparse-display v0.9.0... which satisfies dependency
parse-display = "^0.9"of packagesolang v0.3.3failed to select a version for
proc-macro2which could resolve this conflict