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

Adding update version support #42

Open
wants to merge 16 commits into
base: master
Choose a base branch
from

Conversation

NachoPal
Copy link

@NachoPal NachoPal commented Oct 15, 2023

This PR aims to solve the problem discussed in this forum post: https://forum.polkadot.network/t/publish-substrate-to-crates-io/949/39?u=nachopal

It adds two new options to the update subcommand:

  • --version: this specifies the source of the versions to which the crates will be updated. There are three valid values:
    • latest: it will query the latest crate version from https://crates.io/
    • A URL pointing to a raw Cargo.lock, for example:
      https://raw.githubusercontent.com/paritytech/polkadot-sdk/master/Cargo.lock
    • A path to a local Cargo.lock
  • --exclude: this specifies the path to a TOML file where a [diener_exclude] manifest key is expected, listing the crates we do not want to be updated. (This applies to all kinds of updates, not only for --version)

If it gets approved I can update the README.md

@NachoPal
Copy link
Author

@bkchr Does it need review/approval or I can hit the merge button?

Copy link
Member

@bkchr bkchr left a comment

Choose a reason for hiding this comment

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

Having exclude by passing folders would also be nice.

src/update.rs Outdated
Comment on lines 10 to 17
thread_local! {
/// Packages version - HashMap(name, version)`
pub static PACKAGES_VERSION: RefCell<HashMap<String, String>> = RefCell::new(HashMap::new());
/// Cargo.lock from URL or FILE sources
pub static CARGO_LOCK: RefCell<Option<String>> = RefCell::new(None);
/// Excluded packages from version updating - HashMap(name, exclude)`
pub static EXCLUDED_PACKAGES: RefCell<HashMap<String, bool>> = RefCell::new(HashMap::new());
}
Copy link
Member

Choose a reason for hiding this comment

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

Please no thread locals. Remove them please.

Copy link
Author

Choose a reason for hiding this comment

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

Please no thread locals. Remove them please.

Out of curiosity, is there something wrong about using thread locals? is it a bad practice?

Copy link
Author

Choose a reason for hiding this comment

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

Fixed

src/update.rs Outdated Show resolved Hide resolved
src/update.rs Show resolved Hide resolved
fn get_version_by_source(package: &str, source: &VersionSource) -> Result<String> {
let version = match source {
VersionSource::CratesIO => {
let url = format!("https://crates.io/api/v1/crates/{}", package);
Copy link
Member

Choose a reason for hiding this comment

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

Are you sure that we don't generate too many requests? Maybe we should wait between requests a little bit.

Copy link
Member

@ggwpez ggwpez Oct 25, 2023

Choose a reason for hiding this comment

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

Yes it has limits of one per second. I used index.crates.io which is unlimited here.
The create name needs to be split up, but it is not rate-limited as advantage.

Copy link
Author

Choose a reason for hiding this comment

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

Are you sure that we don't generate too many requests? Maybe we should wait between requests a little bit.

I tested it and it works. Maybe thanks to reqwest::blocking?

@NachoPal
Copy link
Author

NachoPal commented Nov 2, 2023

@bkchr @ggwpez Addressed the comments

In regards

Having exclude by passing folders would also be nice.

Not sure I understand this. Excluding folders under the same workspace would lead to crates version conflicts.

@brenzi
Copy link

brenzi commented Mar 1, 2024

looking forward to this as it simplifies maintenance work for parachain teams.

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.

4 participants