build-image icon indicating copy to clipboard operation
build-image copied to clipboard

fix: fix the builds for public projects that use yarn 2 (or above) with workspaces

Open fulopkovacs opened this issue 2 years ago โ€ข 3 comments

Summary

Fixes #762

The problem it solves

Currently builds are failing for public projects that use yarn 2 (or above) with workspaces. The cached node modules are cached and restored to the wrong location.

The bug

A bit of context: in yarn 1.x only private repos can have workspaces, but this limitation was removed in yarn 2:

Worktrees used to be required to be private (ie list "private": true in their package.json). This requirement got removed with the 2.0 release in order to help standalone projects to progressively adopt workspaces (for example by listing their documentation website as a separate workspace). (source: docs)

The bug is in the run-build-functions.sh file:

  # YARN_IGNORE_PATH will ignore the presence of a local yarn executable (i.e. yarn 2) and default
  # to using the global one (which, for now, is always yarn 1.x). See https://yarnpkg.com/configuration/yarnrc#ignorePath
  # we can actually use this command for npm workspaces as well
  workspace_output="$(YARN_IGNORE_PATH=1 yarn workspaces --json info 2>/dev/null)"
  workspace_exit_code=$?

As you can see it tries to output information about the workspaces with yarn 1.x. in the command substitution and redirects STDERR to /dev/null. After that it only checks if the process exited with 0 or an error code, but ignores the error message, which would tell us, that "workspaces can only be enabled in private projects". Here's an example for the problem from the theatre-js/theatre repo:

  • when the errors are hidden:
 $ YARN_IGNORE_PATH=1 yarn workspaces --json info 2>/dev/null
{"type":"info","data":"Visit \u001b[1mhttps://yarnpkg.com/en/docs/cli/workspaces\u001b[22m for documentation about this command."}
  • when the errors are visible:
$ YARN_IGNORE_PATH=1 yarn workspaces --json info
{"type":"error","data":"Workspaces can only be enabled in private projects."}
{"type":"info","data":"Visit \u001b[1mhttps://yarnpkg.com/en/docs/cli/workspaces\u001b[22m for documentation about this command."}
The solution
  • We can capture the error messages instead of discarding them to check if the problem is related to private workspaces.
  • If it is, then we check the version of yarn that is used in the project and use the new yarn workspaces list --json command to get the information about the workspaces
How I tested the solution

We have a public repo whose builds are failing without clearing the cache after every build. Here's how the cached folders look like with the current build script:

image

The yarn workspaces are not detected in the logs:

image

But here's the previous pictures look like for the fixed build script: image image

I'm not sure if I missed anything, but feel free to tell me if there are things to change! :relaxed:


For us to review and ship your PR efficiently, please perform the following steps:

  • [x] Open a bug/issue before writing your code ๐Ÿง‘โ€๐Ÿ’ป. This ensures we can discuss the changes and get feedback from everyone that should be involved. If you`re fixing a typo or something that`s on fire ๐Ÿ”ฅ (e.g. incident related), you can skip this step.
  • [x] Read the contribution guidelines ๐Ÿ“–. This ensures your code follows our style guide and passes our tests.
  • [x] Update or add tests (if any source code was changed or added) ๐Ÿงช
  • [x] Update the included software doc (if you updated included software) ๐Ÿ“„
  • [x] Update or add documentation (if features were changed or added) ๐Ÿ“
  • [ ] Make sure the status checks below are successful โœ…

A picture of a cute animal (not mandatory, but encouraged) image

fulopkovacs avatar Mar 28 '22 08:03 fulopkovacs

added to backend/support pairing - @audreyshub could you please raise this with the next person you meet with to ask the relevant team to review this for us?

fool avatar Apr 05 '22 18:04 fool

Thank you for your contribution @fulopkovacs! ๐ŸŽ‰ And thanks @fool for raising this. I've added this to our board so we can triage it and prioritise the review next Monday during our planning session.

JGAntunes avatar Apr 08 '22 15:04 JGAntunes

@JGAntunes I finally pass the PR back to review! :grin: There were a few surprises with the tests (e.g.: many of the tests in yarn.bats executed the run_yarn function, which removes the default yarn installation, but there was no proper cleanup after these tests, so the binary for yarn was not available for tests in other files).

One of our biggest pain point gets addressed, if this PR gets merged, but there will still be the problem of caching, which doesn't really work with yarn berry now (35-45s for us for every deploy). Are there any (public) news about that? I saw a PR about it (https://github.com/netlify/build-image/pull/465), but it didn't receive much attention from your side (I didn't check the code of the PR though).

(Btw having worked with the code base and seeing the tests I understand that full support for yarn berry would require quite a bit of planning and effort, so I don't expect you to fix the issue overnight. I would be happy with just a bit more clarity about the place of this feature on the roadmap :relaxed: )

fulopkovacs avatar Jun 06 '22 21:06 fulopkovacs

Support got already implemented inside https://github.com/netlify/build-image/pull/858

lukasholzer avatar Nov 18 '22 09:11 lukasholzer