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.

Sign up for GitHub

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

Jump to bottom

[turborepo] Prune generates invalid pnpm-lock.yaml when injecting local workspace #5529

Closed
gerbyzation opened this issue Jul 14, 2023 · 2 comments · Fixed by #5551
Closed

[turborepo] Prune generates invalid pnpm-lock.yaml when injecting local workspace #5529

gerbyzation opened this issue Jul 14, 2023 · 2 comments · Fixed by #5551
Assignees
@chris-olszewski
Labels
kind: bug Something isn't working

Comments

@gerbyzation
Copy link

gerbyzation commented Jul 14, 2023 •
edited
Loading

What version of Turborepo are you using?

1.10.7

What package manager are you using / does the bug impact?

pnpm

What operating system are you using?

Mac

Describe the Bug

When using turbo prune on a workspace that has a local dependency injected
the generated lockfile has an invalid resolution field. When proceeding to install
the pruned repo this will cause the following error:

❯ pnpm i
Scope: all 3 workspace projects
Lockfile is up to date, resolution step is skipped
Packages: +8
++++++++
 ERR_INVALID_ARG_TYPE  The "path" argument must be of type string. Received undefined

pnpm [ERR_INVALID_ARG_TYPE]: The "path" argument must be of type string. Received undefined
    at new NodeError (node:internal/errors:372:5)
    at validateString (node:internal/validators:120:11)
    at Object.join (node:path:1172:7)
    at directoryFetcher (/Users/gerbenneven/.fnm/node-versions/v16.15.1/installation/lib/node_modules/pnpm/dist/pnpm.cjs:102008:36)
    at fetcher (/Users/gerbenneven/.fnm/node-versions/v16.15.1/installation/lib/node_modules/pnpm/dist/pnpm.cjs:125428:22)
    at ctx.requestsQueue.add.priority.priority (/Users/gerbenneven/.fnm/node-versions/v16.15.1/installation/lib/node_modules/pnpm/dist/pnpm.cjs:125324:78)
    at run (/Users/gerbenneven/.fnm/node-versions/v16.15.1/installation/lib/node_modules/pnpm/dist/pnpm.cjs:124809:90)
    at PQueue._tryToStartAnother (/Users/gerbenneven/.fnm/node-versions/v16.15.1/installation/lib/node_modules/pnpm/dist/pnpm.cjs:124757:13)
    at /Users/gerbenneven/.fnm/node-versions/v16.15.1/installation/lib/node_modules/pnpm/dist/pnpm.cjs:124822:16
    at new Promise (<anonymous>)
Packages are copied from the content-addressable store to the virtual store.
  Content-addressable store is at: /Users/gerbenneven/Library/pnpm/store/v3
  Virtual store is at:             node_modules/.pnpm
Progress: resolved 1, reused 0, downloaded 0, added 0

The original lockfile has the following entry for lib-a:

  file:libraries/lib-b:
    resolution: {directory: libraries/lib-b, type: directory}
    name: lib-b
    version: 1.0.0
    dev: false

The pruned repo generates the following:

  file:libraries/lib-b:
    resolution:
      type: directory
    name: lib-b
    version: 1.0.0
    dev: false

Note the missing directory field on the resolution object.

Expected Behavior

turbo prune should generate a valid lockfile, specifically it shouldn't drop the resolution.directory field on the injected dependency.

To Reproduce

Repository with repro: https://github.com/gerbyzation/turbo-prune-pnpm-resolution-directory-missing-repro
Steps:

pnpm exec turbo prune --scope lib-a
cd out
pnpm install

Reproduction Repo

https://github.com/gerbyzation/turbo-prune-pnpm-resolution-directory-missing-repro
@gerbyzation gerbyzation added area: turborepo kind: bug Something isn't working needs: triage New issues get this label. Remove it after triage labels Jul 14, 2023
@gerbyzation gerbyzation mentioned this issue Jul 14, 2023
Open
@gsoltis gsoltis removed the needs: triage New issues get this label. Remove it after triage label Jul 17, 2023
@chris-olszewski chris-olszewski mentioned this issue Jul 17, 2023
Merged
@chris-olszewski
Copy link
Member

Thanks for the report. #5551 should fix this.

@gerbyzation
Copy link
Author

Great, thanks for the fast turnaround on a fix!

chris-olszewski added a commit that referenced this issue Jul 18, 2023
@chris-olszewski
fix(lockfile): Fix directory resolution variant (#5551)
5ab8ac0 
### Description

Fixes #5529

During the Rust migration I must've messed up the directory field name.
Double checked against
[`@pnpm/lockfile-types`](https://github.com/pnpm/pnpm/blob/main/lockfile/lockfile-types/src/index.ts#L86)
to make sure all of the fields are correct now.

`PackageResolution` should be an enum, but the fact that tarballs are an
untagged variant makes that tricky to communicate to `serde`. A struct
does enough for us.

### Testing Instructions

Added new unit test to make sure we don't lose any fields for the
various variants of the `resolution` field

---------

Co-authored-by: Chris Olszewski <Chris Olszewski>
NicholasLYang pushed a commit that referenced this issue Jul 21, 2023
Squashed commit of the following:
ec8317c 
commit 6c178d2
Author: Alex Kirszenberg <alex.kirszenberg@vercel.com>
Date:   Thu Jul 20 18:06:03 2023 +0200

    Extract shared HMR utils to their own modules/crates (#5576)

    ### Description

    These will also be used in Next.js by the Nexturbo dev API.

    ### Testing Instructions

    N/A

commit e5f43a5
Author: Chris Olszewski <chris.olszewski@vercel.com>
Date:   Wed Jul 19 18:03:12 2023 -0700

    fix: pnpm alias workspace deps (#5569)

    ### Description

    Fixes #5441

    Adds support for [referencing workspaces through
    aliases](https://pnpm.io/workspaces#referencing-workspace-packages-through-aliases)
    by properly resolving them to the correct workspace. Before we would
    mark a package as being an external dependency (or if the alias was a
    valid workspace depend on the incorrect one).

    This PR now recognizes when `workspace:` dependency references a
    different package than the name that's used in the `package.json`.

    Note for reviewers:
    This probably isn't the cleanest solution in either Rust or Go, but
    while we need to maintain two codepaths this keeps the code roughly
    equivalent.

    ### Testing Instructions

    Added unit tests on the Go side.

    Tested manually with a repository where `web` specified it's dependency
    on `@scope/ui` as `"ui": "workspace:@scope/ui@*" and verified that:
    - `turbo run build`: `@scope/ui` finished building before building
    `web`, this hits the Go impl
    - `turbo prune --scope=web`: `@scope/ui` was included in the pruned
    repository, this hits the Rust impl

    ---------

    Co-authored-by: Chris Olszewski <Chris Olszewski>

commit 396bf45
Author: Chris Olszewski <chris.olszewski@vercel.com>
Date:   Wed Jul 19 13:31:35 2023 -0700

    feat: port prune to rust (#5531)

    ### Description

    Port prune from Go to Rust and remove the old Go implementation. To
    achieve this the following was done:
     - Parsing the lockfile during package graph construction
     - Porting utility functions
    - Some minor changes in `turborepo_paths`, these were primarily moving
    methods from their owned to borrowed counterparts
    - Expanding `package.json` parsing to grab some of the information
    pruning requires
    - Porting of the prune command itself. I added some additional structure
    compared to the Go version, but not enough that comparing it to the Go
    version should be difficult

    Notes for reviewers:
    I apologize that this PR ended up touching as much as it did. Reviewing
    the PR by commit should at least make all of the changes and their
    impacts obvious. Commits before `2d4154c` are already on main and can be
    skipped.

    ### Testing Instructions

    Existing unit tests and integration tests for file copying and package
    graph traversal. Actual lockfile behavior is mostly covered by unit
    tests that were ported when the lockfile were ported.

    Also did various manual testing with pruning monorepos.

    ---------

    Co-authored-by: Chris Olszewski <Chris Olszewski>

commit 007b574
Author: Alex Kirszenberg <alex.kirszenberg@vercel.com>
Date:   Wed Jul 19 22:17:39 2023 +0200

    Remove unnecessary ValueDebugFormat item, hide Vc field (#5567)

    ### Description

    This removes unnecessary items from the IDE's autosuggestion.

    I'd also like to get rid of all the `*_inline` suggestions, but RA will
    ignore `#[doc(hidden)]` when inside the same crate. I don't think
    there's a way to indicate "never, ever suggest this item". We could move
    some of these to be private to some generated module, but that module
    would probably still show up as a top level suggestion.

    ### Testing Instructions

    Automated tests.

commit d13b812
Author: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Date:   Wed Jul 19 11:32:07 2023 -0700

    release(turborepo): 1.10.9 (#5565)

    Co-authored-by: Turbobot <turbobot@vercel.com>
    Co-authored-by: Chris Olszewski <Chris Olszewski>

commit 229a2a4
Author: Tobias Koppers <tobias.koppers@googlemail.com>
Date:   Wed Jul 19 16:08:16 2023 +0200

    make with_layer return Vc<Self> (#5563)

    ### Description

    Using the new ability to return `Vc<Self>`

    Co-authored-by: Alex Kirszenberg <alexkirsz@users.noreply.github.com>

commit c78593b
Author: Greg Soltis <greg.soltis@vercel.com>
Date:   Tue Jul 18 20:11:16 2023 -0700

    feat(turborepo): Add proxy support to create-turbo and turbo-gen (#5554)

    Co-authored-by: Greg Soltis <Greg Soltis>

commit 5ab8ac0
Author: Chris Olszewski <chris.olszewski@vercel.com>
Date:   Tue Jul 18 15:06:38 2023 -0700

    fix(lockfile): Fix directory resolution variant (#5551)

    ### Description

    Fixes #5529

    During the Rust migration I must've messed up the directory field name.
    Double checked against
    [`@pnpm/lockfile-types`](https://github.com/pnpm/pnpm/blob/main/lockfile/lockfile-types/src/index.ts#L86)
    to make sure all of the fields are correct now.

    `PackageResolution` should be an enum, but the fact that tarballs are an
    untagged variant makes that tricky to communicate to `serde`. A struct
    does enough for us.

    ### Testing Instructions

    Added new unit test to make sure we don't lose any fields for the
    various variants of the `resolution` field

    ---------

    Co-authored-by: Chris Olszewski <Chris Olszewski>

commit ca7e3e4
Author: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Date:   Tue Jul 18 14:51:34 2023 -0700

    release(turborepo): 1.10.9-canary.0 (#5559)

    Co-authored-by: Turbobot <turbobot@vercel.com>

commit fc5e2b0
Author: Chris Olszewski <chris.olszewski@vercel.com>
Date:   Tue Jul 18 13:16:16 2023 -0700

    chore: no longer convert in relative unix path constructor (#5552)

    ### Description

    To quote @gsoltis:
    > In general:
    > - constructors should validate, to the extent they can (not much for
    `unix` paths, but can verify relative)
    > - conversions should be explicit. You need to know where you're
    starting from. If this were an AnchoredSystemPath on windows, the `\` ->
    `/` makes sense. If it's a literal from e.g. a tar file, then it
    doesn't.

    Reviewers Notes:
    - Opening up this PR in VSCode and using `Find All References` on the
    constructor is useful for double checking that I didn't miss a
    conversion.
    - Clippy error appeared on local when I made these changes. Fixed it
    just in case that would block CI
    - Moved `to_unix` to `AnchoredSystemPath` instead of
    `AnchoredSystemPathBuf` now that we have deref coercion which will
    automatically convert `&AnchroedSystemPathBuf` to `&AnchoredSystemPath`
    and moving the method allows it to be called from either type.

    ### Testing Instructions

    Looked through all uses of `RelativeUnixPathBuf::new` to see if there
    were places that depended on the conversion. The only use that was
    obvious was the usage in `cache_archive/create.rs`. `dotEnv` was the
    only other place where we possibly were converting a system path to a
    relative unix. We don't specify that `dotEnv` entries should be unix
    relative, so we might've been accidentally supporting system paths, but

    ---------

    Co-authored-by: Chris Olszewski <Chris Olszewski>

commit 3eb3a5f
Author: Tobias Koppers <tobias.koppers@googlemail.com>
Date:   Tue Jul 18 21:21:51 2023 +0200

    Ensure output assets reference only output assets (#5557)

    ### Description

    Ensure output assets reference only output assets

    next.js PR: vercel/next.js#52832

commit 22f0bf4
Author: Nathan Hammond <nathan.hammond@vercel.com>
Date:   Wed Jul 19 02:59:12 2023 +0800

    Remove binary optimization step. (#5543)

    Co-authored-by: Nathan Hammond <Nathan Hammond>

commit 5955625
Author: mknichel <7355009+mknichel@users.noreply.github.com>
Date:   Tue Jul 18 10:09:28 2023 -0700

    fix(turborepo): Allow users to select a Vercel team when linking a repository to a Space (#5533)

commit 814f0a2
Author: Leah <github.leah@hrmny.sh>
Date:   Tue Jul 18 17:31:33 2023 +0200

    feat(turbopack-ecmascript): implement acyclic SCC graph for ESM imports (#5506)

commit 46bb9b7
Author: Tobias Koppers <tobias.koppers@googlemail.com>
Date:   Tue Jul 18 13:41:01 2023 +0200

    move references() to specific traits (#5555)

    ### Description

    preparation for making different typed references

    next.js PR: vercel/next.js#52822

commit 56edd9e
Author: Greg Soltis <greg.soltis@vercel.com>
Date:   Mon Jul 17 21:37:26 2023 -0700

    feat(turborepo): Add task ids to failure reports (#5535)

    Co-authored-by: Greg Soltis <Greg Soltis>

commit ea934d1
Author: Greg Soltis <greg.soltis@vercel.com>
Date:   Mon Jul 17 13:52:59 2023 -0700

    chore(turborepo): Turborepo owns the examples-tests directory (#5540)

    Co-authored-by: Greg Soltis <Greg Soltis>

commit 5279282
Author: Alex Kirszenberg <alex.kirszenberg@vercel.com>
Date:   Mon Jul 17 19:55:22 2023 +0200

    Add missing feature to syn (#5547)

    ### Description

    Cargo check currently fails with:

    ```
    error[E0277]: `syn::Type` doesn't implement `Debug`
     --> crates/turbo-tasks-macros-shared/src/primitive_input.rs:8:5
      |
    6 | #[derive(Debug)]
      |          ----- in this derive macro expansion
    7 | pub struct PrimitiveInput {
    8 |     pub ty: Type,
      |     ^^^^^^^^^^^^ `syn::Type` cannot be formatted using `{:?}` because it doesn't implement `Debug`
      |
      = help: the trait `Debug` is not implemented for `syn::Type`
      = note: this error originates in the derive macro `Debug` (in Nightly builds, run with -Z macro-backtrace for more info)

    For more information about this error, try `rustc --explain E0277`.
    error: could not compile `turbo-tasks-macros-shared` (lib) due to previous error
    ```

    This is because because turbo-tasks-macros-shared (which is missing a
    syn feature to enable the debug trait) is part of the workspace default
    members, while turbo-tasks-macros (which has the feature) isn’t.

    ### Testing Instructions

    `cargo check`

commit 78f6cc8
Author: Chris Olszewski <chris.olszewski@vercel.com>
Date:   Mon Jul 17 10:40:13 2023 -0700

    docs: document log order (#5463)

    ### Description

    Adds documentation for `--log-order` #3916 including a callout of our
    special behavior on Github Actions.

    ### Testing Instructions

    Eyes

    ---------

    Co-authored-by: Chris Olszewski <Chris Olszewski>

commit 0771b80
Author: Amit Gurbani <amit1994.gurbani@gmail.com>
Date:   Mon Jul 17 23:09:44 2023 +0530

    Update storybook.mdx (#5537)

    ### Description

    Storybook now can be built with node 18, hence removing this from
    documentation.

    Co-authored-by: Anthony Shew <anthony.shew@vercel.com>

commit 91ca2ae
Author: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Date:   Mon Jul 17 10:11:06 2023 -0700

    release(turborepo): 1.10.8 (#5546)

    Co-authored-by: Turbobot <turbobot@vercel.com>

commit d24075f
Author: Greg Soltis <greg.soltis@vercel.com>
Date:   Mon Jul 17 08:12:38 2023 -0700

    fix(turborepo): Set corepack dir for examples (#5539)

    ### Description

    - set a corepack install directory per example to avoid concurrency
    issues
     - set the `PATH` for the test to include the corepack directory

    ### Testing Instructions

    Examples tests

    ---------

    Co-authored-by: Greg Soltis <Greg Soltis>

commit 46d0945
Author: Nicholas Yang <nicholas.yang@vercel.com>
Date:   Mon Jul 17 11:07:45 2023 -0400

    feat(turborepo): FS Cache (#5473)

    ### Description

    Implements the FS cache on top of CacheItem. ~~This is stacked on top of
    #5065~~

    ### Testing Instructions

    Uses the same round-trip tests of HTTP cache.

    ---------

    Co-authored-by: --global <Nicholas Yang>
    Co-authored-by: Chris Olszewski <chris.olszewski@vercel.com>

commit 135c08f
Author: Tobias Koppers <tobias.koppers@googlemail.com>
Date:   Mon Jul 17 16:44:56 2023 +0200

    add direct cycle detection (#5544)

    ### Description

    very simple detection of dumb mistakes

commit 02f55d9
Author: Tobias Koppers <tobias.koppers@googlemail.com>
Date:   Mon Jul 17 16:33:34 2023 +0200

    move Asset::ident to more specific traits (#5528)

    ### Description

    * `ident()` is no longer on Asset, but on `Module`, `Source`,
    `OutputAsset` or `Chunk`
    * On the way, more AssetVc types needed to be switched to more specific
    traits

    next.js PR: vercel/next.js#52683

commit b069545
Author: Alex Kirszenberg <alex.kirszenberg@vercel.com>
Date:   Mon Jul 17 13:41:18 2023 +0200

    AdjacencyMap::reverse_topological (+ fixes) (#5527)

    ### Description

    This adds `AdjacencyMap::reverse_topological`, which is similar to
    `AdjacencyMap::into_reverse_topological` but doesn't consume the graph.

    This also:
    * Makes `AdjacencyMap` storable in `turbo_tasks::value`s;
    * Fixes ValueDebugFormat and TraceRawVcs derive macros so they support
    generic argument and bounds properly.

    ### Testing Instructions

    N/A

commit 8433a32
Author: Alex Kirszenberg <alex.kirszenberg@vercel.com>
Date:   Sun Jul 16 14:07:49 2023 +0200

    Vc<T> and Turbo Engine type system improvements (#4587)

    This PR changes our Turbo Engine code generation from generating
    additional `TypeVc` types to allowing the `Vc<Type>` notation. It also
    brings other improvements to the Turbo Engine type system, like more
    type-safe downcasting and upcasting, better support for primitives, the
    possibility to accept `&self` in `#[value_impl]` implementations
    everywhere, and a bunch of other changes.

    link WEB-379

commit fdc358a
Author: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Date:   Fri Jul 14 20:31:07 2023 -0700

    release(turborepo): 1.10.8-canary.2 (#5534)

    Co-authored-by: Turbobot <turbobot@vercel.com>

commit 26fee25
Author: Greg Soltis <greg.soltis@vercel.com>
Date:   Fri Jul 14 11:13:59 2023 -0700

    fix(turborepo): Export and match on our copy of BasicUI (#5532)

    Co-authored-by: Greg Soltis <Greg Soltis>

commit b0ea0a8
Author: Greg Soltis <greg.soltis@vercel.com>
Date:   Fri Jul 14 10:39:52 2023 -0700

    fix(turborepo): Rebuild turbo if Go code has changed (#5530)

    Co-authored-by: Greg Soltis <Greg Soltis>

commit b6bb8fe
Author: Chris Olszewski <chris.olszewski@vercel.com>
Date:   Fri Jul 14 06:47:13 2023 -0700

    chore: use fs-err in turborepo fs related libs (#5517)

    ### Description
    Swaps our usage of various `fs` methods to use `fs-err` instead. To
    quote the `fs-err` docs:

    > Using [std::fs](https://doc.rust-lang.org/stable/std/fs/), if this
    code fails:
    >
    > `let file = File::open("does not exist.txt")?;`
    >
    > The error message that Rust gives you isn't very useful:
    >
    > `The system cannot find the file specified. (os error 2)`
    >
    > ...but if we use `fs-err` instead, our error contains more actionable
    information:
    >
    > ```failed to open file `does not exist.txt`
    > caused by: The system cannot find the file specified. (os error 2)```

    ### Testing Instructions
    Existing unit tests pass

    Co-authored-by: Chris Olszewski <Chris Olszewski>

commit f3a36e7
Author: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Date:   Thu Jul 13 15:27:21 2023 -0700

    release(turborepo): 1.10.8-canary.1 (#5523)

    Co-authored-by: Turbobot <turbobot@vercel.com>

commit cabca3c
Author: Nicholas Yang <nicholas.yang@vercel.com>
Date:   Thu Jul 13 16:12:46 2023 -0400

    chore: Extend examples timeout (#5504)

    ### Description

    We get a lot of timeout failures on examples tests. Since this almost
    always requires re-running the test, which adds another 30 minutes of
    compute time, it's probably better to extend the timeout to 40 minutes.

    ### Testing Instructions

    <!--
      Give a quick description of steps to test your changes.
    -->

    Co-authored-by: nicholaslyang <Nicholas Yang>

commit 7c18c80
Author: Greg Soltis <greg.soltis@vercel.com>
Date:   Thu Jul 13 12:58:14 2023 -0700

    Implement hashing fallback (#5505)

    Co-authored-by: Greg Soltis <Greg Soltis>

commit 67e71c7
Author: Anthony Shew <anthony.shew@vercel.com>
Date:   Thu Jul 13 11:35:08 2023 -0700

    Better spot for link. (#5520)

commit a14180e
Author: Anthony Shew <anthony.shew@vercel.com>
Date:   Thu Jul 13 11:24:26 2023 -0700

    Fix link. (#5518)

commit b7aaa7b
Author: Nicholas Yang <nicholas.yang@vercel.com>
Date:   Thu Jul 13 13:18:44 2023 -0400

    chore: Added clippy deny all to crates (#5514)

    Co-authored-by: nicholaslyang <Nicholas Yang>

commit e3c68fa
Author: Alex Kirszenberg <alex.kirszenberg@vercel.com>
Date:   Thu Jul 13 16:34:27 2023 +0200

    Add any_content_changed_of_output_assets (#5513)

    ### Description

    Required for vercel/next.js#52259

    ### Testing Instructions

    N/A

    Co-authored-by: Tobias Koppers <sokra@users.noreply.github.com>

commit 4022f2b
Author: Tobias Koppers <tobias.koppers@googlemail.com>
Date:   Thu Jul 13 14:54:55 2023 +0200

    OutputAsset trait (#5507)

    ### Description

    adding a trait to all assets in the output graph

    next.js PR: vercel/next.js#52606
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@chris-olszewski chris-olszewski

Labels
kind: bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

fix(lockfile): Fix directory resolution variant vercel/turborepo
3 participants
@gsoltis @gerbyzation @chris-olszewski