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

language/node: add setup_yarn_environment #2962

Closed
wants to merge 1 commit into from

Conversation

chrmoritz
Copy link
Contributor

@chrmoritz chrmoritz commented Jul 28, 2017

  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same change?
  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your changes? Here's an example.
  • Have you successfully run brew tests with your changes locally?

This PR adds a setup_yarn_environment helper method to language/node, which will be needed for the upcoming yarn v0.28 (currently in brew install yarn --devel) to be able to build some yarn depending formulae.

What it does:

  1. It writes a temporary .yarnrc to .brew_home to tell yarn too:
    • prefix: set's yarns global prefix to .brew_home/.yarn/bin so that global build tool installations won't fail because of brew sandbox restrictions (like auto installing node-gyp on demand)
    • cache-folder: changes yarn's cache folder to HOMEBREW_CACHE/yarn_cache (from .brew_home/Library/Caches/Yarn) similar to what we do for npm
    • build-from-source: compile native addons from source instead of downloading prebuild binaries
    • ignore-engines: in yarn 0.28+ installing with a different node version than specified in the modules package.json engine filed would fail otherwise (warning => error)
  2. It adds said temporary global prefix to the $PATH
  3. It adds the node-gyp copy of the homebrew managed npm to the $PATH as a workaround preventing yarn from having to globally install a temporary copy of node-gyp on demand (yarn default detecting node-gyp from npm code does not work because of homebrews-npm-installation-structure-oddness)

yarnrc = Pathname.new("#{ENV["HOME"]}/.yarnrc")
# only run setup_yarn_environment once per formula
return if yarnrc.exist?
# Writing a temporary `.yarnrc` to `.brew_home` to tell yarn to set it's global prefix
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/it's/its/

@ilovezfs
Copy link
Contributor

ignore-engines: in yarn 0.28+ installing with a different node version than specified in the modules package.json engine filed would fail otherwise (warning => error)

Can you please elaborate on the problem? Given this isn't a released stable version yet, it might be possible to work with upstream so that we don't need a back-end workaround here.

yarn default detecting node-gyp from npm code does not work because of homebrews-npm-installation-structure-oddness

Can we make the installation less "odd" so that this isn't necessary? It seems weird if we're installing node in such a way that the modules cannot be found by yarn without back-end changes to brew.

@chrmoritz
Copy link
Contributor Author

chrmoritz commented Jul 28, 2017

  1. It was introduced in 0.28.0 in Include top-level package.json in engines check yarnpkg/yarn#3675 and is more or less only a consequent bug fixing to enforce strict engine checks also for the top level module and not only for all dependencies (which is done in yarn for quite some time now). Later is a debatable behavior with lots of issues in yarns repo complaining about it, but it was done on purpose see: Soft Engines Check yarnpkg/rfcs#69 (comment).
  2. Yarn is expecting a default node installation and is looking for the node-gyp binary based on the default relative path from the node executable. This isn't fixable in homebrew unless we move npm out of libexec, which I wouldn't recommend.

Fixed comment type and fixed test to not write to ~/.yarnrc.

@ilovezfs
Copy link
Contributor

enforce strict engine checks

Does that mean requiring a specific version of yarn?

This isn't fixable in homebrew unless we move npm out of libexec, which I wouldn't recommend.

would it work to put a link to node in libexec/bin?

@chrmoritz
Copy link
Contributor Author

chrmoritz commented Jul 28, 2017

Does that mean requiring a specific version of yarn?

It means requiring a specific version of node (sry if I missed to explain that). For example metabase requires node to be exactly v4.4.7 and yarn will throw an error in yarn v0.28+ (instead of only warning about it) if the build tool is run with a different version (even if it works fine with other node versions too). BTW: Npm did move in the exact opposite direction and deprecated the engine filed and does not even warn anymore about it in recent versions.

would it work to put a link to node in libexec/bin?

Added a link to the actual code above. We would need to link node-gyp-bin to either HOMEBREW_PREFIX/Cellar/node/<version>/bin/node_modules/npm/bin/node-gyp-bin or HOMEBREW_PREFIX/Cellar/node/<version>/lib/node_modules/npm/bin/node-gyp-bin from its current location HOMEBREW_PREFIX/Cellar/node/<version>/libexec/lib/node_modules/npm/bin/node-gyp-bin to make it work.

Also this piece of code is a optional performance optimization. Because we've set yarns global prefix to something sandbox writeable and added it to the PATH we could also simply rely on yarn's fallback to temporary download node-gyp to yarns global prefix on-demand every time we install a formula.

Edit: If I'm going to upstream something to yarn anyway, maybe I can get this fix in too: chrmoritz/yarn@fb440c9 (which adds support for detecting node-gyp from a homebrew managed npm in yarn)

@chrmoritz
Copy link
Contributor Author

chrmoritz commented Jul 28, 2017

I can confirm that node-gyp detection works in homebrew without the last 5 lines with my latest yarn test build including chrmoritz/yarn@fb440c9.

I've also added setting yarn's cache to HOMEBREW_CACHE/yarn_cache (from .brew_home/Library/Caches/Yarn) similar to what we do for npm to this PR.

@MikeMcQuaid
Copy link
Member

As a general note I think pretty much all the language/* modules have made formulae worse. They make it opaque to formula authors as to what is being run, when and why. A formula should make it clear when looked at what commands can be run be a user in the terminal to produce the same end-results.

Honestly if we have to write all this code to support yarn it's significantly more tempting to say that we just don't support anything that requires yarn. You could write similar levels of automagic for things that use gem but we've avoided doing that.

@chrmoritz
Copy link
Contributor Author

The only thing which is required to support yarn 0.28+ is the prefix and ignore-engines from yarnrc. The other fields (cache-folder and build-from-source) are for making it fit better into the homebrew environment. And the node-gyp fix should go away once I get my fix upstreamed.

@MikeMcQuaid
Copy link
Member

If we're going to install yarn globally we should configure it globally, too. If that means patching it rather than adding stuff into Homebrew/brew that seems preferable.

@chrmoritz
Copy link
Contributor Author

And how should this be done in case of prefix. For users of the yarn formula it should point to something accessible (in the $PATH for a default homebrew installation) like HOMEBREW_PREFIX/bin, but during installation of formulas using yarn it has to point to something writeable in the homebrew sandbox and HOMEBREW_PREFIX/bin is not writeable during install (only during post_install).

@ilovezfs
Copy link
Contributor

And how should this be done in case of prefix

It can be done

  1. the same way it's done here, but duplicated in each formula
  2. with a separate env script wrapper
  3. by string replacing the engine lock to our version of node
  4. maybe some other way(s)

@ilovezfs
Copy link
Contributor

Another idea about prefix: this seems like something yarn could accept as a command line switch.

@chrmoritz
Copy link
Contributor Author

  1. the same way it's done here, but duplicated in each formula

I thought the point of having these language helpers here is to avoid such duplication for language specific common task. In theory we could remove all language helpers and duplicate their functionality in all formulae as needed. But this would quickly end up in a maintenance nightmare, because every time we would need to make only the slightest change to such an helper method, we would need to make it to a lot of formulas at the same time and make sure to not forget a formula (which has failed in the past, see Homebrew/homebrew-core#16074 for example).

  1. with a separate env script wrapper

You mean writing a build specific env script wrapper to buildpath and prepending it to the PATH before the actual yarn bin from cellar/bin? This would work with a lot of code duplication (see 1), but wouldn't fix ignore-engines because there is no env var for this.

  1. by string replacing the engine lock to our version of node

This would fix ignore-engines, but not prefix.

Another idea about prefix: this seems like something yarn could accept as a command line switch.

It does, --prefix= and --ignore-engine are valid cmdline switches. But the reason why I created a setup_yarn_environment and not a local_yarn_install_args method is, that in 3 of 4 yarn (as build tool) using formulas in core yarn is not called directly in the formula, but somewhere deep inside their build scripts instead (we can't directly pass cmdline switches to yarn). Using these cmdline switches to setup the correct environment for homebrews build environment would therefore require us to modify their buildscripts to make use of these switches and carry this as a patch indefinitely, which should be avoided as you said in the other issue.

@ilovezfs
Copy link
Contributor

You mean writing a build specific env script wrapper to buildpath and prepending it to the PATH before the actual yarn bin from cellar/bin? This would work with a lot of code duplication (see 1), but wouldn't fix ignore-engines because there is no env var for this.

It could set PREFIX from some ENV["YARN_PREFIX"] = buildpath at the top of a formula.

@ilovezfs
Copy link
Contributor

i.e. we could adjust the env script wrapper to first check whether YARN_PREFIX is set, and if so use that, else use PREFIX=#{HOMEBREW_PREFIX} as the current one does.

@chrmoritz
Copy link
Contributor Author

But I thought the plan is to get rid of the formulae env script wrappers again once we update to a yarn version, which includes a fix for the yarn global prefix != HOMEBREW_PREFIX issue.

@MikeMcQuaid
Copy link
Member

I think a goal of having no env script wrappers would be preferable to having env script wrappers which would be preferable to temporary inreplaces which would be preferable to permanent inreplaces which would be preferable to Homebrew/brew changes we have to support pretty much forever.

@chrmoritz
Copy link
Contributor Author

I think a goal of having no env script wrappers would be preferable to having env script wrappers which would be preferable to temporary inreplaces which would be preferable to permanent inreplaces which would be preferable to Homebrew/brew changes we have to support pretty much forever.

So you would prefer:

  • Shipping an ugly env wrapper script forever with yarn, which normally just redirects calls directly to the yarn executable (we assume the fix landed upstream) and only if a HOMEBREW_YARN_ENV variable is set it adds --prefix=HOMEBREW_PREFIX --ignore-engines to the commands. Also you would have to set ENV["HOMEBREW_YARN_ENV"] = 1 in every yarn using formula.

over:

  • Shipping again only normal libexec symlinks (like before) with yarn and have to write a .yarnrc file in every yarn using formula with Pathname.new("#{ENV["HOME"]}/.yarnrc").write "prefix \"#{HOMEBREW_PREFIX}\"\nignore-engines true".

?

@MikeMcQuaid
Copy link
Member

@chrmoritz I've expressed many times my preference is working with upstream so that we don't need custom code to handle all this stuff, honestly.

@chrmoritz
Copy link
Contributor Author

And how should yarn upstream be able to solve the issue that:

  • HOMEBREW_PREFIX/bin is not writeable during install with the sandbox enabled
  • or to which other prefix should global yarn modules be installed which is both install sandbox writable and suitable for global cmdline tool installation (in the users path)
  • and the fact that we build formulas with our latest node version, even if their upstream official requires a different node version (even if there are no issue with the latest node)?

@MikeMcQuaid
Copy link
Member

HOMEBREW_PREFIX/bin is not writeable during install with the sandbox enabled

The same way all software does: just write to the Cellar and resolve symlinks accordingly and, if really needed (which most software doesn't, notably) allow that directory to be specified in another argument to a configure equivalent.

or to which other prefix should global yarn modules be installed which is both install sandbox writable and suitable for global cmdline tool installation (in the users path)

The same way Ruby/Python do: allow configuration of where things can be installed which doesn't infer needing write permissions there at install time.

even if there are no issue with the latest node

So I'm not sure what the question/problem is?

@MikeMcQuaid
Copy link
Member

I mean, let's be clear here: yarn (and maybe npm too) is the exception to the vast majority of software we package. It doesn't need to be this hard and it these don't need to be things that every package manager needs to solve separately.

@chrmoritz
Copy link
Contributor Author

chrmoritz commented Jul 29, 2017

@MikeMcQuaid I think you misunderstood my last comment. I was talking about using yarn as a build tool inside another formula (the case this PR tries to fix) and not about installing yarn itself.

@MikeMcQuaid
Copy link
Member

@chrmoritz I understand that. We don't need this level of handholding for most buildsystems, even.

@chrmoritz
Copy link
Contributor Author

Ok, but than:

allow that directory to be specified in another argument to a configure equivalent.

This would need to be handled by e.g. grafana and not by yarn: grafana would need to add a configure argument to set yarns prefix and it should support newer node version in their engine field too.

So I'm not sure what the question/problem is?

It's about strict engine checks, which is not an issue with yarn, but either an issue with upstream being way to strict/conservative about supported node version or an issue with the homebrew formula not using the exact node version upstream wants (I'm not saying that doing this is a good idea, we have enough pain with it for kibana)

@MikeMcQuaid
Copy link
Member

It's about strict engine checks, which is not an issue with yarn, but either an issue with upstream being way to strict/conservative about supported node version or an issue with the homebrew formula not using the exact node version upstream wants (I'm not saying that doing this is a good idea, we have enough pain with it for kibana)

Again this increasingly seems to point towards the "we stop packaging software that can't play nicely with Homebrew" as the solution.

@ilovezfs
Copy link
Contributor

I don't see yarn as being particularly more difficult to make play nice with Homebrew than anything else.

Given we currently have a grand total of three formulae that build using yarn, it is way too early to conclude either that we need a DSL or that we should not package them.

@stale stale bot added the stale No recent activity label Aug 22, 2017
@stale
Copy link

stale bot commented Aug 22, 2017

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

@ilovezfs ilovezfs added the in progress Maintainers are working on this label Aug 27, 2017
@stale stale bot removed the stale No recent activity label Aug 27, 2017
@MikeMcQuaid
Copy link
Member

@chrmoritz What's the status of this?

@MikeMcQuaid
Copy link
Member

@chrmoritz Closing for now but happy to reopen when you let us know the status.

@Homebrew Homebrew locked and limited conversation to collaborators May 4, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
in progress Maintainers are working on this
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants