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

feat: get versions from the repo directly #11

Merged
merged 2 commits into from
Apr 17, 2024
Merged

Conversation

patriciobcs
Copy link
Collaborator

@patriciobcs patriciobcs commented Apr 16, 2024

Solves: #9

This changes the tool workflow, reducing to almost none its maintenance. Now, it will always fetch the versions directly from the repo using raw files. It seems the rate limit is 5000 queries per hour (ref). Considering the tool will max fetch 2 times (when falling back to Cargo.lock). The rate-limit is totally acceptable. If anyone would want to have a greater limit, in the future this can be replace by direct API usage by using GitHub CLI or API tokens.

@patriciobcs patriciobcs requested a review from ggwpez April 16, 2024 15:26
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated
```

### Offline

You can use PSVM offline with the `-v` or `--version` flag, cause PSVM keeps a local mapping (i.e. [v1.3.0 Mapping](/src/versions/release-crates-io-v1.3.0.json)) of the Polkadot SDK dependencies versions, that will be slowly updated. So be aware that the latest Polkadot SDK versions may not be available in the local mapping.
Copy link
Member

Choose a reason for hiding this comment

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

Sure that we need the offline version at all? As soon as there are two ways to do something and one is wrong, then ppl will end up doing the wrong thing and complain.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I keep it to just not cause a breaking change with the "offline" justification, but probably is useless cause anyways you need internet to download the crates after setting a version.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The only concern here is the fact that some branches don't have a Plan.toml file for example. What do you about fallback to the Cargo.lock in case Plan.toml not found. For example v1.3.0: https://raw.githubusercontent.com/paritytech/polkadot-sdk/release-crates-io-v1.3.0/Plan.toml

Copy link
Member

Choose a reason for hiding this comment

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

Yea the Cargo.lock fallback is good.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ok! It's done, also added a test to check if it works for all release branches, and it works! The only branch without Plan.toml is v1.3.0, but in this case it will fallback to Cargo.lock. Added a warning in the Workflow section.

@patriciobcs patriciobcs requested a review from ggwpez April 17, 2024 11:27
@patriciobcs patriciobcs changed the title feat: get branch cargo.lock or plan.toml versions feat: get versions from the repo directly Apr 17, 2024
Copy link
Member

@ggwpez ggwpez left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@patriciobcs patriciobcs merged commit 1969933 into main Apr 17, 2024
1 check passed
@rzadp
Copy link
Contributor

rzadp commented Apr 29, 2024

Hey @patriciobcs, thanks for this change!

I wonder, what is the reason to use Plan.toml in the first place, and then Cargo.lock as a fallback - instead of only using Cargo.lock?

I'm asking because I'm having some issues with some of the crates when using Plan.toml in the recent (1.9.0 and 1.10.0 releases) - specifically frame / polkadot-sdk-frame, and substrate-build-script-utils.

In my case, Cargo.lock still works, so I wonder what's the rationale for preferring Plan.toml.

@patriciobcs
Copy link
Collaborator Author

Hey @patriciobcs, thanks for this change!

I wonder, what is the reason to use Plan.toml in the first place, and then Cargo.lock as a fallback - instead of only using Cargo.lock?

I'm asking because I'm having some issues with some of the crates when using Plan.toml in the recent (1.9.0 and 1.10.0 releases) - specifically frame / polkadot-sdk-frame, and substrate-build-script-utils.

In my case, Cargo.lock still works, so I wonder what's the rationale for preferring Plan.toml.

The Cargo.lock file may contain dependencies that are not published in crates.io, and the tool will not be able to filter them out cause it is not possible to determine if a crate is published or not (with this file). If you have a local dependency with a name similar to a crate not published, the tool will overwrite it, so be careful. Adding an extra argument to use directly the Cargo.lock can also be done, but be aware of this issue.

@rzadp
Copy link
Contributor

rzadp commented May 2, 2024

Thanks @patriciobcs

I see what you mean.

The Cargo.lock file may contain dependencies that are not published in crates.io, and the tool will not be able to filter them out cause it is not possible to determine if a crate is published or not (with this file).

That is true. However, with Plan.toml I'm running into an exactly opposite problem:

The Plan.toml file may contain dependencies that are published in crates.io, but marked as publish = false. And the tool is not able to distinguish them from dependencies that are truly not published (within this file).

How so? The Plan.toml marks publish = false crates that are not supposed to be published - that includes both private crates, and public crates that have not changed (so don't need to be uploaded again without changes).

Example:

psvm filters them both out, which is wrong.


I can think of three solutions to this:

  • Make a query to crates.io for every crate marked with publish=false, see if the relevant version is published or not.
  • Same as above, but make a query to github for Cargo.toml file (not sure if we can find a path to it just looking at Plan.toml).
  • Remove this filter.
    • This is not ideal because it only flips the problem to the other opposite, but it's the simplest thing that would unblock my work.
    • At least the user would get an understandable error when installing dependencies after using psvm - that a crate they're trying to use is not available, as opposed to now - psvm silently failing to update some dependencies.

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.

None yet

3 participants