Skip to content
This repository has been archived by the owner on Jan 15, 2025. It is now read-only.

Add monotrail-utils crate #53

Merged
merged 19 commits into from
Jun 12, 2023
Merged

Add monotrail-utils crate #53

merged 19 commits into from
Jun 12, 2023

Conversation

cnpryer
Copy link
Contributor

@cnpryer cnpryer commented Jun 6, 2023

UPDATED:

This PR moves RequirementsTxt to a new monotrail-utils crate.

Summary of changes:

  1. Add monotrail-utils crate
  2. Move RequirementsTxt over
  3. Use RequirementsTxt in poetry_integration

Tag astral-sh/rye#265
Closes #56

[dependencies]
anyhow = "1.0.65"
fs-err = "2.8.1"
pep508_rs = { git = "https://github.com/konstin/pep508_rs", rev = "df87d4ff0f0f3554780ab82680539cb190b0a585", features = ["serde"] }
Copy link
Owner

Choose a reason for hiding this comment

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

Can this use a released version?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added in d64b333

@@ -0,0 +1,3 @@
pub use requirements_txt::RequirementsTxt;
Copy link
Owner

Choose a reason for hiding this comment

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

This should get some module level (or rather crate level) doc comment about what this crate is

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do we want to hold off on fleshing it out? Added in 97987ad

@konstin
Copy link
Owner

konstin commented Jun 7, 2023

Move the poetry integration usage into monotrail-utils

Could you use the monotrail_util RequirementsTxt in the poetry integration?

We also need updated CI workflows to publish monotrail-utils to crates.io, not necessarily now but before it blocks downstream crates from publishing to crates.io

@cnpryer
Copy link
Contributor Author

cnpryer commented Jun 7, 2023

Could you use the monotrail_util RequirementsTxt in the poetry integration?

I'll check it out.

@cnpryer cnpryer marked this pull request as draft June 7, 2023 17:54
@cnpryer cnpryer marked this pull request as ready for review June 7, 2023 21:44
@cnpryer
Copy link
Contributor Author

cnpryer commented Jun 8, 2023

I can look into making a PR for the CI after if that's alright with you.

I'm planning on getting astral-sh/rye#265 ready for a review next. Then I can loop around to the CI.

Comment on lines -539 to +540
let requirements = RequirementsTxt::parse(&requirements_txt, &current_dir()?)?
.into_poetry(&requirements_txt)?;
let requirements = read_requirements_for_poetry(&requirements_txt, &current_dir()?)?;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe I'm adding an allocation here.

Copy link
Owner

Choose a reason for hiding this comment

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

tbh i'm not to concerned about allocations here, i don't think requirements.txt parsing will take any noticeable amount of time, especially when compared to network requests, deserialization and package installation (those three i've seen as bottlenecks in flamegraphs)

Copy link
Owner

@konstin konstin left a comment

Choose a reason for hiding this comment

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

cargo test --workspace and cargo clippy --tests --all-features --workspace are still failing, otherwise looks good

Comment on lines 7 to 14
anyhow = "1.0.65"
fs-err = "2.8.1"
pep508_rs = { version = "0.2.1", features = ["serde"] }
serde = { version = "1.0.145", features = ["derive"] }
serde_json = "1.0.85"
toml = "0.7.2"
tracing = "0.1.36"
unscanny = "0.1.0"
Copy link
Owner

Choose a reason for hiding this comment

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

Here we can inherit the versions for the shared dependencies from the workspace root: https://doc.rust-lang.org/cargo/reference/workspaces.html#the-package-table

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would you like to use shared deps in install-wheel-rs? I can make that a follow up PR if you want.

Copy link
Owner

Choose a reason for hiding this comment

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

that's be great!

Copy link
Contributor Author

@cnpryer cnpryer Jun 10, 2023

Choose a reason for hiding this comment

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

I couldn't find anything about dev deps. Started with 175f7b3. Done

Copy link
Contributor Author

Choose a reason for hiding this comment

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

IIUC I'd want to use [workspace.dependencies] for dev dependencies as well. Added in ee7a182

@cnpryer
Copy link
Contributor Author

cnpryer commented Jun 10, 2023

I have the same 12 pass 1 failure (wheel::test::installed_paths_relative) on main for cargo test --workspace. I'll look again later.

UPDATE: #56

@cnpryer cnpryer marked this pull request as draft June 10, 2023 17:09
@cnpryer
Copy link
Contributor Author

cnpryer commented Jun 10, 2023

I can create an issue for this, but on main cargo test --release results with

failures:
    test_flipstring
    test_pipx_black_version
    test_poetry_config
    test_srcery

I'm on macos.

UPDATE: #55

@cnpryer cnpryer marked this pull request as ready for review June 10, 2023 22:01
@konstin
Copy link
Owner

konstin commented Jun 12, 2023

Thanks!

@konstin konstin merged commit 596c51e into konstin:main Jun 12, 2023
@cnpryer cnpryer deleted the monotrail-utils branch June 12, 2023 19:09
@cnpryer cnpryer mentioned this pull request Jun 13, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

requirements_txt::test::test_empty_file fails on MacOS
2 participants