-
Notifications
You must be signed in to change notification settings - Fork 117
feat: Implementation Paths V2 #2032
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
base: main
Are you sure you want to change the base?
Conversation
baszalmstra
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is going in the right direction.
I do think we can make this change backwards compatible. Now it isnt because the version in paths.json is bumped. This means that other installers will most likely fail with these packages. That will hamper adoption.
Instead perhaps we can keep the version the same. The offsets field is already backwards compatible. But the executable flag is not because we dont know if the absence of the field means nothing is executable or it was not written. Maybe instead we can add another field to the top-level keys to indicate that the executable field should be taken into account.
crates/rattler/src/install/link.rs
Outdated
| &target_prefix, | ||
| &target_platform, | ||
| *file_mode, | ||
| offsets.to_vec() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cant we keep this as a slice instead of allocating on the heap?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes! that seems like a way prettier solution indeed
| MetadataMissing, | ||
|
|
||
| /// An error occurred with the version of the `path.json` file | ||
| #[error("")] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you improve this error?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did improve it but think I am not using it anywhere, Depending on whatever leads out of the v1/v2 comment you left before I might even remove it
| /// When a file is executable this will be true | ||
| /// This entry is only present in version 2 of the paths.json file | ||
| #[serde(default, skip_serializing_if = "Option::is_none")] | ||
| pub executable: Option<bool>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You did not implement this yet right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did not really understand what improvements were the goal in adding the executability to the paths, because of this I didn't yet implement them no
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense. Let me explain. At the moment the code has to acquire the metadata in the cache to determine if the file has the executable permissions set. If so, then the file also needs to have it set in the environment. However, this metadata call performs a syscall which adds a bit of overhead. Using the executable flag in the paths.json we can skip this metadata check.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
makes a lot more sense now! I think I now implemented this with the new commit
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not entirely. What we try to avoid is the have to query the file metadata all together. You currently implementation still requests this regardless of whether executable was set.
But what Im trying to avoid is having to bump the version field in paths.json. Because doing so will break installers that aren't compatible with this version (e.g. conda and mamba). We need to make sure that adding the executable field is a backwards compatible change. Simply adding a field is backwards compatible but then if the field is optional we dont know if the absence of the fields means either the value is false or that the value was not computed in the first place.
To fix that we could add another top-level key (next to version) that indicates that the absence of the executable field means it should be considered false. Then, if that top-level field is present we no longer have to read the metadata from disk and we can rely on the per-file executable field.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah correct, I hope that the current adaptation is what you meant!
I was still retrieving the metadata because it seems to be needed for the file permissions and timestamps,
should there be another solution for retrieving this metadata or does this mean we keep the current approach?
crates/rattler/src/install/link.rs
Outdated
| let old_len = segment.len(); | ||
|
|
||
| let mut out = Vec::with_capacity(old_len); | ||
| let mut remaining = segment; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like this inner loop could be simplified. The code know exactly where the offsets are and how long they are. I dont think we need the finder or the Vec. This seems very much like the code from the previous implementation, I encourage you to rethink it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did indeed not push that which I had been working on, in the new commit I did.
crates/rattler/src/install/link.rs
Outdated
| if offsets.is_empty() { | ||
| return destination.write_all(source_bytes); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems redundant. If the offsets is empty, the loop will have no iterations and at the end of the function the rest of the bytes will be written in a similar fashion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right!
improved permission checking, improved error message, improved slice usage in link.rs, Still have to look at the v2 backwards incompatibility comment
The absence of the field would inherently mean it was not written otherwise it would read as true or false, I guess you assumed that it would, like the prefix_placeholder, be left empty if false. this currently isn't the case but something like the following would be the preference? |
Description
First implementation for issue #1814 asking for a paths.json version 2 support
Implementing deserialisation for the proposed paths version 2, would love to hear some feedback where possible.
What happened
I added the field for a file being executable but did not implement any implementations with this.
I also added the offsets field for which I implemented new prefix replacement functions
What should still be done/ next steps
A next step would be the implementation of version 2 paths.json when building a package with rattler-build & a possible CEP