Skip to content
This repository was archived by the owner on Sep 17, 2023. It is now read-only.

Commit f894194

Browse files
committed
fix(make-depends): do not delete npm pack archive
Since `mv` "deletes" the source file on disk, we have a race condition where two actors are trying to build the same tarball. Consider two tasks, t_a and t_b. The optimal happy path looks like 1. t_a runs `npm pack`, creating the tarball in the package dir 2. t_a runs `mv` to move the tarball into `/dist` 3. t_b sees the tarball in `/dist` and takes no action A less happy path can occur, due to the length of time `npm pack` can take (~1s) 1. t_a runs `npm pack`, creating the tarball in the package dir 2. t_b runs `npm pack`, creating the tarball in the package dir 3. t_a runs `mv` to move the tarball into `/dist` 4. t_b runs `mv` to move the tarball into `/dist` <-- error! Step 4 here will fail with an error like ``` mv: can't rename 'packages/my-package/scoped-my-package-1.2.3.tgz': No such file or directory ``` because the first `mv` removes the tarball from disk. This brings into question the sanity of using a top-level `/dist` in the first place! Ideally we'd atomically write the tarball into `/dist` with npm 7's pack-destination[^1], but this flag is not present in npm 6. TODO: remove the `/dist` convention all-together [^1]: https://docs.npmjs.com/cli/v7/commands/npm-pack#pack-destination
1 parent 6fefb40 commit f894194

File tree

1 file changed

+1
-1
lines changed

1 file changed

+1
-1
lines changed

templates/makefile

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -27,7 +27,7 @@ $({{ unscoped_package_name.replace("-", "_").to_uppercase() }}_INTERNAL_DEPENDEN
2727
# NOTE: when npm 7+ supports a pack-destination so when npm 6 is EOL we should migrate to that
2828
dist/{{ scoped_package_name.trim_start_matches("@").replace("/", "-") }}.tgz: | dist
2929
cd {{ package_directory }}; npm pack
30-
mv {{ package_directory }}/{{ scoped_package_name.trim_start_matches("@").replace("/", "-") }}-$({{ unscoped_package_name.replace("-", "_").to_uppercase() }}_VERSION).tgz $@
30+
ln {{ package_directory }}/{{ scoped_package_name.trim_start_matches("@").replace("/", "-") }}-$({{ unscoped_package_name.replace("-", "_").to_uppercase() }}_VERSION).tgz $@
3131

3232
{{ unscoped_package_name.replace("-", "_").to_uppercase() }}_INTERNAL_NPM_DEPENDENCIES = {{ exclusive_transitive_internal_dependency_npm_pack_archives.join(" \\\n") }}
3333

0 commit comments

Comments
 (0)