fix: better out-of source caching and source code propagation #4875
Conversation
| /// | ||
| /// This is useful for the case where you want to checkout a source that is at | ||
| /// different location than the manifest, most obviously in an out-of-tree build. | ||
| pub async fn checkout_source_location( |
There was a problem hiding this comment.
Use this method when wanting to check out the actual source.
| alternative_root: Option<PinnedSourceSpec>, | ||
| ) -> Result<SourceCheckout, CommandDispatcherError<SourceCheckoutError>> { | ||
| match pinned_spec { | ||
| PinnedSourceSpec::Path(ref path) => { | ||
| PinnedSourceSpec::Path(path_spec) => { | ||
| let alternative_root_path = match alternative_root.as_ref() { | ||
| Some(PinnedSourceSpec::Path(alt_path)) => Some( | ||
| self.data | ||
| .resolve_typed_path(alt_path.path.to_path(), None) | ||
| .map_err(|e| CommandDispatcherError::Failed(e.into()))?, | ||
| ), | ||
| _ => None, | ||
| }; | ||
|
|
There was a problem hiding this comment.
This basically fixes the PlotJuggler bug, because code will be checked relative to the package manifest instead of the workspace manifest.
tdejager
changed the title
crates/pixi_command_dispatcher/src/build_backend_metadata/mod.rs
Outdated
Show resolved
Hide resolved
| match pinned_spec { | ||
| PinnedSourceSpec::Path(ref path) => { | ||
| PinnedSourceSpec::Path(path_spec) => { | ||
| let alternative_root_path = match alternative_root.as_ref() { |
There was a problem hiding this comment.
I think this can be replaced with SourceAnchor! Its purpose is to make one sourcespec relative to another.
You can also have a git source with a subdirectory for instance. I dont think the current implementation deals with that?
There was a problem hiding this comment.
I think it does because there is a test for it, in the build suite and that will never be interpreted as relative.
But I didn't know that had that purpose, should I make the change and also update SourceAnchor documentation?
There was a problem hiding this comment.
Yes please! If you dont have time I can also do it!
|
I changed the code significantly. I'm trying to make sure that the pinned @tdejager What is the best way to test this? |
|
There was a PR on the suite I made: prefix-dev/pixi-build-testsuite#91 And for a real-world test you can try my PR on PlotJuggler. Can find the link if you want but I'm on mobile. Note that there are also some unit test in pixi that test parts of this, but does not call any builds. |
|
I changed the code to always make the |
|
I looked at the changes :) is now the preferred_source an alternative to the build_source or is it only used for the pinning during a pixi update? |
| /// TODO: Currently this doesn't use workspace_root and just resolves build_source | ||
| /// relative to manifest_source. Future implementation should resolve both against | ||
| /// workspace_root first. |
There was a problem hiding this comment.
This still needs implementing. The idea is: PinnedSourceSpec is always relative to the workspace root. But the build_source in the lock-file is relative to the location of the package. We need to convert these two. My reasoning was that this can only reliably be done if all relative paths have a base so you can compare apples to apples. The workspace_root should provide this base.
| /// TODO: Currently this doesn't use workspace_root and just makes build_source | ||
| /// relative to manifest_source. Future implementation should make both relative | ||
| /// to workspace_root first. |
There was a problem hiding this comment.
This still needs to happen. See my other comments.
4 participants
This adds the
SourceCodeLocationstruct that holds both the location of the manifest and the potential build-source, which is generally the actual source-code itself.E.g given this pixi.toml (imagine it being part of a workspace with a pixi.toml):
workspace/proj/pixi.tomlThen we get:
manifest_source=workspace/projbuild_source=./barWhich when called using the
source_code()method gives you:workspace/proj/bar, in this specific case.We can use this structure when to pass around and decide if we need the location to the manifest, or to the source code. Like when caching we need to check the source code to see if we need to do rebuilds. For example, in my plotjugger PR constant rebuilds would occur: facontidavide/PlotJuggler#1201. This PR also fixes that behavior.
AI Disclosure
SourceCodeLocation, I let codex then propagate that struct throughout the codebase.