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

Support editable in pip-sync and pip-compile #587

Merged
merged 10 commits into from
Dec 16, 2023

Conversation

konstin
Copy link
Member

@konstin konstin commented Dec 7, 2023

Support -e path/do/dir in pip-sync and and pip-compile.

@konstin konstin force-pushed the konstin/activate-venv-before-build branch from b36697a to 97a0452 Compare December 12, 2023 14:34
Base automatically changed from konstin/activate-venv-before-build to main December 12, 2023 14:46
@konstin konstin force-pushed the konstin/editable-in-pip-sync branch 2 times, most recently from 5ebd9b4 to b62d97f Compare December 12, 2023 17:52
@konstin konstin force-pushed the konstin/editable-in-pip-sync branch 2 times, most recently from 517202b to e9ee271 Compare December 13, 2023 13:17
konstin added a commit that referenced this pull request Dec 13, 2023
Split out to minimize the diff in #587
konstin added a commit that referenced this pull request Dec 13, 2023
Split out to minimize the diff in #587
@konstin konstin force-pushed the konstin/editable-in-pip-sync branch 2 times, most recently from 25cbfca to f8b889d Compare December 13, 2023 14:12
@konstin konstin marked this pull request as ready for review December 13, 2023 22:33
@konstin
Copy link
Member Author

konstin commented Dec 13, 2023

The most important things work: We build editable wheel, extract metadata or install them, we build them in parallel, we report their building progress, we only audit them if they are installed. I would try to merge this and open issues for everything else, there are merge conflicts again and i only solved a big bunch of them already.

let bars = self.bars.lock().unwrap();
let progress = &bars[index];
progress.finish_with_message(format!(
" {} {}",
"Built".bold().green(),
dist.to_color_string()
dist.to_string().dimmed()
Copy link
Member

Choose a reason for hiding this comment

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

Why this change? I don't think this is the same. In the existing code, only the version is dimmed.

Copy link
Member

Choose a reason for hiding this comment

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

I think this is still incorrect.

@charliermarsh
Copy link
Member

I will fix all of these merge conflicts, and I may even take some of the smaller changes here and commit them as separate PRs to help move this along.

allowed_urls: manifest
.requirements
.iter()
.chain(manifest.constraints.iter())
.filter_map(|req| {
if let Some(pep508_rs::VersionOrUrl::Url(url)) = &req.version_or_url {
Some(url)
Some(url.clone())
Copy link
Member

Choose a reason for hiding this comment

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

Why does this now need to be cloned? Can we avoid this?

Copy link
Member

Choose a reason for hiding this comment

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

Like CanonicalUrl::new only needs a borrowed representation, so it seems unnecessary.

Copy link
Member Author

Choose a reason for hiding this comment

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

We'd have to rewrite the signature of AllowedUrls, if i just use &Url then editable.url() doesn't live long enough

@charliermarsh
Copy link
Member

@konstin - Did you a solid and fixed all the merge conflicts...

# This file was autogenerated by Puffin v0.0.1 via the following command:
# puffin pip-compile requirements.in --cache-dir [CACHE_DIR]
boltons==23.1.1
-e ../../scripts/editable-installs/maturin_editable
Copy link
Member

Choose a reason for hiding this comment

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

I think this test introduces a dependency on Maturin. Or, at least, I can't run this test locally, so we need to fix the setup.

Copy link
Member Author

Choose a reason for hiding this comment

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

That's a bug! Could you share the error?

Copy link
Member

Choose a reason for hiding this comment

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

          5 │+error: Failed to build editables␊
          6 │+  Caused by: Failed to build editable: ../../scripts/editable-installs/maturin_editable␊
          7 │+  Caused by: Failed to build: ../../scripts/editable-installs/maturin_editable␊
          8 │+  Caused by: Build backend failed to build wheel through `build_editable()`:␊
          9 │+--- stdout:␊
         10 │+Running `maturin pep517 build-wheel -i /private/var/folders/nt/6gf2v7_s3k13zq_t3944rwz40000gn/T/.tmpM0ztUQ/.venv/bin/python --compatibility off --editable`␊
         11 │+--- stderr:␊
         12 │+🍹 Building a mixed python/rust project␊
         13 │+🔗 Found pyo3 bindings␊
         14 │+🐍 Found CPython 3.12 at /private/var/folders/nt/6gf2v7_s3k13zq_t3944rwz40000gn/T/.tmpM0ztUQ/.venv/bin/python␊
         15 │+📡 Using build options features from pyproject.toml␊
         16 │+   Compiling target-lexicon v0.12.12␊
         17 │+   Compiling autocfg v1.1.0␊
         18 │+   Compiling proc-macro2 v1.0.70␊
         19 │+   Compiling once_cell v1.18.0␊
         20 │+   Compiling libc v0.2.150␊
         21 │+   Compiling unicode-ident v1.0.12␊
         22 │+   Compiling parking_lot_core v0.9.9␊
         23 │+   Compiling heck v0.4.1␊
         24 │+   Compiling cfg-if v1.0.0␊
         25 │+   Compiling scopeguard v1.2.0␊
         26 │+   Compiling smallvec v1.11.2␊
         27 │+   Compiling unindent v0.2.3␊
         28 │+   Compiling indoc v2.0.4␊
         29 │+   Compiling lock_api v0.4.11␊
         30 │+   Compiling memoffset v0.9.0␊
         31 │+   Compiling quote v1.0.33␊
         32 │+   Compiling syn v2.0.39␊
         33 │+   Compiling pyo3-build-config v0.20.0␊
         34 │+   Compiling parking_lot v0.12.1␊
         35 │+   Compiling pyo3-ffi v0.20.0␊
         36 │+   Compiling pyo3 v0.20.0␊
         37 │+   Compiling pyo3-macros-backend v0.20.0␊
         38 │+   Compiling pyo3-macros v0.20.0␊
         39 │+   Compiling maturin_editable v0.1.0 (/Users/crmarsh/workspace/puffin/scripts/editable-installs/maturin_editable)␊
         40 │+error: linking with `clang` failed: exit status: 1␊
         41 │+  |␊
         42 │+  = note: env -u IPHONEOS_DEPLOYMENT_TARGET -u TVOS_DEPLOYMENT_TARGET LC_ALL="C" PATH="/Users/crmarsh/.rustup/toolchains/1.74-aarch64-apple-darwin/lib/rustlib/aarch64-apple-darwin/bin:/private/var/folders/nt/6gf2v7_s3k13zq_t3944rwz40000gn/T/.tmpM0ztUQ/.venv/bin:/Users/crmarsh/.local/share/rtx/installs/python/3.10.13/bin:/Users/crmarsh/workspace/puffin/target/release:/Users/crmarsh/.bun/bin:/opt/homebrew/sbin:/opt/homebrew/bin:/Library/Frameworks/Python.framework/Versions/3.7/bin:/usr/local/bin:/usr/bin:/bin:/usr/sbin:/sbin:/Users/crmarsh/.cargo/bin:/Users/crmarsh/.deno/bin:/Users/crmarsh/.poetry/bin:/Users/crmarsh/.pyenv/bin:/Users/crmarsh/bin:/opt/homebrew/bin:/Users/crmarsh/.local/bin:/Users/crmarsh/.antigen/bundles/agkozak/zsh-z:/Users/crmarsh/.antigen/bundles/zsh-users/zsh-syntax-highlighting:/Users/crmarsh/.antigen/bundles/zsh-users/zsh-autosuggestions:/Users/crmarsh/.local/bin" VSLANG="1033" ZERO_AR_DATE="1" "clang" "-Wl,-exported_symbols_list,/var/folders/nt/6gf2v7_s3k13zq_t3944rwz40000gn/T/rustc0PS1h0/list" "-arch" "arm64" "/var/folders/nt/6gf2v7_s3k13zq_t3944rwz40000gn/T/rustc0PS1h0/symbols.o" "/Users/crmarsh/workspace/puffin/scripts/editable-installs/maturin_editable/target/release/deps/maturin_editable.maturin_editable.91ec885a940e6035-cgu.0.rcgu.o" "/Users/crmarsh/workspace/puffin/scripts/editable-installs/maturin_editable/target/release/deps/maturin_editable.maturin_editable.91ec885a940e6035-cgu.1.rcgu.o" "/Users/crmarsh/workspace/puffin/scripts/editable-installs/maturin_editable/target/release/deps/maturin_editable.maturin_editable.91ec885a940e6035-cgu.2.rcgu.o" "/Users/crmarsh/workspace/puffin/scripts/editable-installs/maturin_editable/target/release/deps/maturin_editable.4a42g8ucu8w57flt.rcgu.o" "-L" "/Users/crmarsh/workspace/puffin/scripts/editable-installs/maturin_editable/target/release/deps" "-L" "/Users/crmarsh/.rustup/toolchains/1.74-aarch64-apple-darwin/lib/rustlib/aarch64-apple-darwin/lib" "/Users/crmarsh/workspace/puffin/scripts/editable-installs/maturin_editable/target/release/deps/libpyo3-23ac21fa9877a14c.rlib" "/Users/crmarsh/workspace/puffin/scripts/editable-installs/maturin_editable/target/release/deps/libmemoffset-fd46ffa8dc916e95.rlib" "/Users/crmarsh/workspace/puffin/scripts/editable-installs/maturin_editable/target/release/deps/libparking_lot-c122df108af6f399.rlib" "/Users/crmarsh/workspace/puffin/scripts/editable-installs/maturin_editable/target/release/deps/libparking_lot_core-5995d18fcd96ee3c.rlib" "/Users/crmarsh/workspace/puffin/scripts/editable-installs/maturin_editable/target/release/deps/libcfg_if-52840a58dc691991.rlib" "/Users/crmarsh/workspace/puffin/scripts/editable-installs/maturin_editable/target/release/deps/libsmallvec-00ff269f207bba05.rlib" "/Users/crmarsh/workspace/puffin/scripts/editable-installs/maturin_editable/target/release/deps/liblock_api-f71e1f3e27acdcca.rlib" "/Users/crmarsh/workspace/puffin/scripts/editable-installs/maturin_editable/target/release/deps/libscopeguard-edcdd31530886fe0.rlib" "/Users/crmarsh/workspace/puffin/scripts/editable-installs/maturin_editable/target/release/deps/libpyo3_ffi-a83a66c6dcf71c21.rlib" "/Users/crmarsh/workspace/puffin/scripts/editable-installs/maturin_editable/target/release/deps/liblibc-137b43a52fa183f9.rlib" "/Users/crmarsh/workspace/puffin/scripts/editable-installs/maturin_editable/target/release/deps/libunindent-f17976f67c75cbf5.rlib" "/Users/crmarsh/.rustup/toolchains/1.74-aarch64-apple-darwin/lib/rustlib/aarch64-apple-darwin/lib/libstd-5563368f93f04a18.rlib" "/Users/crmarsh/.rustup/toolchains/1.74-aarch64-apple-darwin/lib/rustlib/aarch64-apple-darwin/lib/libpanic_unwind-56e96ebffd3d9808.rlib" "/Users/crmarsh/.rustup/toolchains/1.74-aarch64-apple-darwin/lib/rustlib/aarch64-apple-darwin/lib/libobject-68ad5facd2da3c54.rlib" "/Users/crmarsh/.rustup/toolchains/1.74-aarch64-apple-darwin/lib/rustlib/aarch64-apple-darwin/lib/libmemchr-ed648c021defb5b4.rlib" "/Users/crmarsh/.rustup/toolchains/1.74-aarch64-apple-darwin/lib/rustlib/aarch64-apple-darwin/lib/libaddr2line-815db56da00be265.rlib" "/Users/crmarsh/.rustup/toolchains/1.74-aarch64-apple-darwin/lib/rustlib/aarch64-apple-darwin/lib/libgimli-5186709c031b65af.rlib" "/Users/crmarsh/.rustup/toolchains/1.74-aarch64-apple-darwin/lib/rustlib/aarch64-apple-darwin/lib/librustc_demangle-7cb2a31ae866e369.rlib" "/Users/crmarsh/.rustup/toolchains/1.74-aarch64-apple-darwin/lib/rustlib/aarch64-apple-darwin/lib/libstd_detect-c39e8cee81fb9ad0.rlib" "/Users/crmarsh/.rustup/toolchains/1.74-aarch64-apple-darwin/lib/rustlib/aarch64-apple-darwin/lib/libhashbrown-b8aeb6382a15b7e5.rlib" "/Users/crmarsh/.rustup/toolchains/1.74-aarch64-apple-darwin/lib/rustlib/aarch64-apple-darwin/lib/librustc_std_workspace_alloc-152de6c346c443c1.rlib" "/Users/crmarsh/.rustup/toolchains/1.74-aarch64-apple-darwin/lib/rustlib/aarch64-apple-darwin/lib/libminiz_oxide-274e1083efe4f227.rlib" "/Users/crmarsh/.rustup/toolchains/1.74-aarch64-apple-darwin/lib/rustlib/aarch64-apple-darwin/lib/libadler-519dc439ccb69841.rlib" "/Users/crmarsh/.rustup/toolchains/1.74-aarch64-apple-darwin/lib/rustlib/aarch64-apple-darwin/lib/libunwind-24c437e0616b2003.rlib" "/Users/crmarsh/.rustup/toolchains/1.74-aarch64-apple-darwin/lib/rustlib/aarch64-apple-darwin/lib/libcfg_if-70eb1def4bb8ab07.rlib" "/Users/crmarsh/.rustup/toolchains/1.74-aarch64-apple-darwin/lib/rustlib/aarch64-apple-darwin/lib/liblibc-9c748d96a757609c.rlib" "/Users/crmarsh/.rustup/toolchains/1.74-aarch64-apple-darwin/lib/rustlib/aarch64-apple-darwin/lib/liballoc-7543628317133907.rlib" "/Users/crmarsh/.rustup/toolchains/1.74-aarch64-apple-darwin/lib/rustlib/aarch64-apple-darwin/lib/librustc_std_workspace_core-8af68f47e6f26d40.rlib" "/Users/crmarsh/.rustup/toolchains/1.74-aarch64-apple-darwin/lib/rustlib/aarch64-apple-darwin/lib/libcore-a60a966a64bff48d.rlib" "/Users/crmarsh/.rustup/toolchains/1.74-aarch64-apple-darwin/lib/rustlib/aarch64-apple-darwin/lib/libcompiler_builtins-eeccd9f755247d6f.rlib" "-liconv" "-lSystem" "-lc" "-lm" "-L" "/Users/crmarsh/.rustup/toolchains/1.74-aarch64-apple-darwin/lib/rustlib/aarch64-apple-darwin/lib" "-o" "/Users/crmarsh/workspace/puffin/scripts/editable-installs/maturin_editable/target/release/deps/libmaturin_editable.dylib" "-Wl,-dead_strip" "-dynamiclib" "-Wl,-dylib" "-nodefaultlibs" "-undefined" "dynamic_lookup" "-Wl,-install_name,@rpath/maturin_editable.cpython-312-darwin.so" "-fuse-ld=/Users/crmarsh/workspace/sold/build/ld64"␊
         43 │+  = note: mold: fatal: unknown command line option: -undefined␊
         44 │+          clang: error: linker command failed with exit code 1 (use -v to see invocation)␊
         45 │+          ␊
         46 │+␊
         47 │+error: could not compile `maturin_editable` (lib) due to previous error␊
         48 │+💥 maturin failed␊
         49 │+  Caused by: Failed to build a native library through cargo␊
         50 │+  Caused by: Cargo build finished with "exit status: 101": `CARGO_ENCODED_RUSTFLAGS="-C\u{1f}link-arg=-fuse-ld=/Users/crmarsh/workspace/sold/build/ld64" PYO3_ENVIRONMENT_SIGNATURE="cpython-3.12-64bit" PYO3_PYTHON="/private/var/folders/nt/6gf2v7_s3k13zq_t3944rwz40000gn/T/.tmpM0ztUQ/.venv/bin/python" PYTHON_SYS_EXECUTABLE="/private/var/folders/nt/6gf2v7_s3k13zq_t3944rwz40000gn/T/.tmpM0ztUQ/.venv/bin/python" "cargo" "rustc" "--features" "pyo3/extension-module" "--message-format" "json-render-diagnostics" "--manifest-path" "/Users/crmarsh/workspace/puffin/scripts/editable-installs/maturin_editable/Cargo.toml" "--release" "--lib" "--" "-C" "link-arg=-undefined" "-C" "link-arg=dynamic_lookup" "-C" "link-args=-Wl,-install_name,@rpath/maturin_editable.cpython-312-darwin.so"`␊
         51 │+Error: command ['maturin', 'pep517', 'build-wheel', '-i', '/private/var/folders/nt/6gf2v7_s3k13zq_t3944rwz40000gn/T/.tmpM0ztUQ/.venv/bin/python', '--compatibility', 'off', '--editable'] returned non-zero exit status 1␊
         52 │+---␊

Copy link
Member

Choose a reason for hiding this comment

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

This I do need to figure out... blerg.

Copy link
Member

Choose a reason for hiding this comment

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

It works without mold (sigh). Not sure why?

Copy link
Member Author

Choose a reason for hiding this comment

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

The undefined is a build option we have to set on macos for pyo3 or extension-modules will not compile, afaik because they use libpython symbols while not linking against libpython.

Can you try a build with maturin on the cli? This looks like it could be a bug in maturin or pyo3.

@konstin
Copy link
Member Author

konstin commented Dec 14, 2023

Rebased onto main. I've started using VerbatimUrl for editables too, though not in path parsing yet.

Downloaded 2 packages in [TIME]
Installed 4 packages in [TIME]
+ boltons==23.1.1
+ maturin-editable @ ../../scripts/editable-installs/maturin_editable
Copy link
Member Author

Choose a reason for hiding this comment

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

I believe it would be better to show the resolved absolute path here instead of the original, it helps with debugging.

Copy link
Member

Choose a reason for hiding this comment

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

That's reasonable but it'll require at least one new abstraction since this uses Display, and Display for VerbatimUrl uses the given string. So we need another trait.

Copy link
Member Author

Choose a reason for hiding this comment

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

What about original() -> str and resolved() -> &str in dist? They feel like Debug and Display but i wouldn't want to use Debug for either.

Copy link
Member

Choose a reason for hiding this comment

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

I would call original either verbatim or given but it sounds roughly correct yes. The problem is that these are implements on the distribution types (like SourceDist), so we need a separate method on that entire hierarchy.

Copy link
Member Author

Choose a reason for hiding this comment

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

Will do in a follow up PR

overrides,
preferences,
project,
Vec::new(),
Copy link
Member

Choose a reason for hiding this comment

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

So this doesn't support editables yet? Should there be a TODO?

.editables
.iter()
.map(|(editable, _)| editable.url().raw().clone()),
)
Copy link
Member

Choose a reason for hiding this comment

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

I believe the clones and change to take an owned reference here can and should be avoided.

}
}
}

/// Uses an [`Arc`] internally, clone freely
#[derive(Debug, Default, Clone)]
pub struct SourceBuildContext {
Copy link
Member

Choose a reason for hiding this comment

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

I know it's crazy, but is it guaranteed that the build requirements don't include an editable package? Right now, we seem to assume so.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, the build requirements are pep 508 Requirements which don't have an editable variant.

Copy link
Member

Choose a reason for hiding this comment

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

Phew!

@charliermarsh
Copy link
Member

If I find time, I'll try to handle these last few followups and merge since this keeps hitting conflicts :)

elapsed(start.elapsed())
)
.dimmed()
)?;
Copy link
Member

Choose a reason for hiding this comment

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

Isn't the reuse of the Downloader across invocations with text in-between going to mess with the progress bar?

@charliermarsh charliermarsh enabled auto-merge (squash) December 16, 2023 22:33
@charliermarsh
Copy link
Member

Okay, I may push some small follow-ups too but it seems best to get this merged. Let's go!

@charliermarsh charliermarsh merged commit f059c6e into main Dec 16, 2023
3 checks passed
@charliermarsh charliermarsh deleted the konstin/editable-in-pip-sync branch December 16, 2023 22:37
@konstin
Copy link
Member Author

konstin commented Dec 17, 2023

Thank you for finishing this!

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

2 participants