-
Notifications
You must be signed in to change notification settings - Fork 21
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
feat: store dir structure compatible with pnpm #166
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #166 +/- ##
==========================================
+ Coverage 86.08% 86.58% +0.49%
==========================================
Files 53 56 +3
Lines 2710 2863 +153
==========================================
+ Hits 2333 2479 +146
- Misses 377 384 +7
☔ View full report in Codecov by Sentry. |
Micro-Benchmark ResultsLinux
|
Integrated-Benchmark Report (Linux)Scenario: Frozen Lockfile
|
crates/store-dir/src/index_file.rs
Outdated
/// Path to an index file of a tarball. | ||
pub fn tarball_index_file_path(&self, tarball_integrity: &Integrity) -> PathBuf { | ||
let (algorithm, hex) = tarball_integrity.to_hex(); | ||
assert_eq!(algorithm, Algorithm::Sha512, "Only Sha512 is supported"); // TODO: propagate 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.
It is handled by the ssri library, which can accept any SHA version and convert it to hex. I assume it is just a shorter hex because sha1 is shorter.
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.
Why do we need so many files? Isn't it enough to test on a package that has two files? One executable and one non-executable?
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.
If you can suggest a package that has only a few files with one executable, I will use 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.
There relevant code is here:
pacquet/crates/cli/tests/install.rs
Lines 57 to 61 in 95741ef
let package_json_content = serde_json::json!({ | |
"dependencies": { | |
"pretty-exec": "0.3.10", | |
}, | |
}); |
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.
We have a registry-mock with testing packages. It has a package with one executable: https://github.com/pnpm/registry-mock/tree/main/packages/hello-world-js-bin%401.0.0
We just need to start using the registry-mock
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.
We just need to start using the registry-mock
This should be in another PR.
There's one important nuance to linking from the store. This shouldn't be done in this PR but we should remember to do it. When a package that has postinstall scripts is "imported" from the store, it is only imported using reflinks or copying. This is important because we build the package and during build the files could get modified. We don't want files in the store become modified. Also, we will have to implement side-effects caching. But that's also for a separate PR and not high priority at the moment. |
Resolves #165
Other changes:
pacquet-store-dir
has been created.store-dir
inNpmrc
has been changed fromPathBuf
toStoreDir
.write_sync
inpacquet-cafs
has been replaced by 2 functions (8f98e73).pacquet-cafs
crate has been merged intopacquet-store-dir
(db673ef).pacquet-tarball
crate now also writes index files (3b6e68a..8f98e73).pacquet prune
now panics ontodo!()
for being incomplete (c8526d0).TODO:
cas_file_path
.index_file_path
.pnpm install --offline
to test store dir compatibility.sha2::Sha512
.prune
(c8526d0).checkedAt
.cafs
.StoreDir::new
and switch all.to_path_buf().into()
to using it.cafs
intostore-dir
(db673ef).From
toTaskError
.write_sync
into 2 functions, one for non-index, one for index (8f98e73).TarballIndex
tostore-dir
and reuse it inwrite_tarball_index_file
.file_path_by_content_address
should takeexec: bool
instead ofsuffix: FileSuffix
.FileSuffix
andstrum
.pacquet_fs::make_file_executable
.