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.

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

🗂 Cache outputs outside package folder not caching #944

Closed
ckortekaas opened this issue Mar 25, 2022 · 3 comments
Closed

🗂 Cache outputs outside package folder not caching #944

ckortekaas opened this issue Mar 25, 2022 · 3 comments
Assignees
Labels
kind: bug Something isn't working

Comments

@ckortekaas
Copy link

ckortekaas commented Mar 25, 2022

What version of Turborepo are you using?

1.1.10 (problem also persists in 1.2.1)

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

pnpm

What operating system are you using?

Mac

Describe the Bug

For example a '@orgname/api' package that lives off the monorepo root directory in a /api folder, setting the top-level turbo.json outputs to:

    "@orgname/api#build": {
      "dependsOn": ["^build"],
      "outputs": ["../build/@orgname/api/**"]
    },

It just doesn't cache it. I've tried various permutations (including in the default ^build outputs instead of with # scopes) but caching only works for relative paths inside the same folder (as in previous releases).

There are no errors in the build logs (with -vvv on) and the log has the named ../build/@orgname/api/** path in the list of dirs it says it will cache.

The node_modules/.cache/turbo has a hash dir for the folder (eg /api/.turbo) but it only contains a turbo-build.log file. Other packages with relative output settings (that don't use ../ paths) cache output targets all ok.

Is there something more that needs to be added to the file path Go code to allow it to use the ../ (or ../../ and deeper) syntax?

FWIW, this is using Node 16.14.2 on MacOS 12.3 with latest PNPM and using PNPM Workspaces, running pnpm turbo run build from the root dir of the monorepo.

Just wondering if to make ../ or ../../ style outputs paths work for caching below the current package dir, do they need to be wrapped in filepath.Abs first to translate it to an absolute path? Eg in the Put function for CopyOrLinkFile to work? Maybe those Go functions only work with absolute paths so any relative paths need translations first?

This is related to #497 and #861

Expected Behavior

Built code a package creates outside the package dir gets put into Turborepo cache.

To Reproduce

  1. Specify an outputs location as a relative path below the current package directory eg:
"outputs": ["../build/@orgname/api/**"]
  1. Run a turbo repo build
  2. Look in node_modules/.cache/turbo and see only turbo-build.log not the cached folders that you see when you cache a dir inside the package path.
@jaredpalmer jaredpalmer added the kind: bug Something isn't working label Apr 11, 2022
@gsoltis
Copy link
Contributor

gsoltis commented Apr 14, 2022

I can confirm this issue. We need to update our cache.Put logic to handle paths outside of a particular package.

@ckortekaas
Copy link
Author

I've tested and this is fixed in 1.2.8. Thanks @nathanhammond and @gsoltis 🙇‍♂️

@nathanhammond
Copy link
Contributor

Heya @ckortekaas! Thanks for your patience and following along as we're getting these sorts of edge cases all cleared up. I do actually need to go back in to this and add a warning to this as it's a bit dangerous to cache something that isn't explicitly an output of the current execution node or its parents as we provide no guarantees about order of execution.

That doesn't mean that it won't work, but you'll get me looking at you like this: 🤨. Please take extra care to make sure that you have a controlled execution order (by way of dependencies or command invocation order).

One possible thing to be aware of: we currently repopulate the directories from the cache before proceeding to child nodes but that's a place where we can "cheat" a little bit and move the I/O to a background thread that only has to sync back before we cross over to a child node that depends on it. I have no reason to believe that we won't still support your usage pattern but it could eliminate one optimization that we can make to the build (if we can't programmatically reason about what it is doing).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind: bug Something isn't working
Projects
None yet
Development

No branches or pull requests

4 participants