-
-
Notifications
You must be signed in to change notification settings - Fork 12.5k
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
yarn: fix to not use npm to install #11881
Conversation
Ah, figured it out! The clue was in the last one. The old symlinks pointed directly to the Probably because the symlink check in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This has 42,055 installs in the last 30 days. I'd prefer we do such "experiments" on something a bit more obscure than this.
Not sure how this is "an experiment"? It exchanges an unsupported way of installing yarn with a supported one. Aren't the tests to be trusted? And the fact that npm is used to install broke the installation from brew for me yesterday since I got a new npm from a newer node. So even though the tarball itself was unchanged, I was unable to use a brew-installed yarn without resetting brew to a place in history with another node version. EDIT: This also makes yarn take up 13MB instead of 19MB since it just includes what has to be there, and not the extra files |
That could describe a huge number of formulae, so not really convincing.
Not sure I understand what that means but it's not really relevant to this specific PR and sounds like something you should open an issue about. |
inreplace "#{libexec}/lib/node_modules/yarn/package.json", '"installationMethod": "tar"', '"installationMethod": "homebrew"' | ||
libexec.install Dir["*"] | ||
bin.install_symlink "#{libexec}/bin/yarn.js" => "yarn" | ||
bin.install_symlink "#{libexec}/bin/yarn.js" => "yarnpkg" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pretty much my only objection here is if they add additional commands that we now need to remember to update/manually add them. Thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Those are extracted from the bin
object in package.json. Is there some easy way to use something like jq
to extract it?
$ cat package.json | jq '.bin'
{
"yarn": "./bin/yarn.js",
"yarnpkg": "./bin/yarn.js"
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could use Ruby's JSON support to do so but it worries me a little if we get to the point that we're parsing these files rather than letting npm
or similar do so...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note this is handled without any additional fuss by the current formula.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That said, I doubt they'll add new ones. yarn
is the name of the thing, but since some debian distros already have a global executable called yarn
,yarnpkg
is added as an alias. @Daniel15 can probably comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general @Daniel15 having very strong feelings on this PR either way would be valued input.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I doubt we'll add additional executables other than yarn
and yarnpkg
so I think it's fine to just hard-code those two. I don't know enough about Homebrew to have strong feelings either way.
The executables are in a bin
directory, could Homebrew just link all the extensionless files in the bin
directory? https://github.com/yarnpkg/yarn/tree/master/bin
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Daniel15 That's what I had at the start (well, I linked all files in bin
, but still), but the yarn
executable in there was unable to properly follow symlinks (or it followed symlinks it shouldn't, not sure?), so I ended in the situation I describe in the OP
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
$ readlink /usr/local/bin/yarn
../Cellar/yarn/0.21.3/bin/yarn
$ realpath /usr/local/bin/yarn
/usr/local/Cellar/yarn/0.21.3/libexec/bin/yarn.js
Can realpath
be trusted to exist on the platforms that runs that shellscript? readlink -f
isn't available on Mac
Although yarnpkg
just runs yarn.js
, it doesn't do the stuff yarn
does. Is that by design?
@MikeMcQuaid Changed the symlinking to read paths and binaries from package.json |
That seems overly complex given what Daniel said above. |
Like I replied there, it doesn't work. That was the whole trouble I had in the OP, since I just symlinked the whole bin folder. In addition to non-working scripts, I also got extra files linked. I can filter those out, or just use a whitelist, but then it isn't really any less complex, IMO 😄 |
I would hard code the list not parse json. |
The code in this PR seems reasonable to me. It's up to the Homebrew maintainers though :) |
Formula/yarn.rb
Outdated
bin.install_symlink Dir["#{libexec}/bin/*"] | ||
inreplace "#{libexec}/lib/node_modules/yarn/package.json", '"installationMethod": "tar"', '"installationMethod": "homebrew"' | ||
libexec.install Dir["*"] | ||
package_json = JSON.parse File.read("#{libexec}/package.json") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we're going down this road of parsing these json packaging files ourselves, which I don't think is advisable, it would belong in the brew backend with a DSL, not proliferated throughout formulae.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That I can agree on, I was actually thinking the exact same thing 😄
Yeah hardcoding |
This. |
That's what I had. And again, the extensionless files in If it's ok to hardcode to |
@SimenB Yes, but now we have new information since Daniel said, "I doubt we'll add additional executables other than yarn and yarnpkg so I think it's fine to just hard-code those two." |
@SimenB Here's the Debian package script, for reference: https://github.com/yarnpkg/yarn/blob/fda53e97b07d32982d9ada611588d4baedc7ff51/scripts/build-deb.sh#L57-L59. In the Debian package, we symlink |
Cool, I'll revert the changes I did today 😄 @Daniel15 No, Basically yarnpkg/yarn#2511 |
Oh yeah, I always forget that Mac uses BSD tools rather than GNU tools 😛 If you like, you could modify Yarn's |
I can try sending a PR for it sure, but then we'd have to hold off on this PR until a new (stable) release of yarn. No biggie of course 😄 |
Yeah it would be great to have a BSD friendly shell script. Note that
(unless you've installed |
Assuming upstream indicated an intent to accept that PR, we could always apply it as a patch here. |
We can merge this linking yarn.js and if we can make the |
Yes, whatever works properly without adding unnecessary dependencies! :) |
Ok, removed the changes involving package.json |
This seems fine to me as long as it's well tested before it goes in. |
I have tested it on my personal Mac and the one I use at work. Is there any more testing I can do? |
@SimenB You could try it with #11991 which wants to use |
Lumo worked fine. Grafana had some issue. I don't think it's the installed yarn's fault, though, as I get the same error with the yarn installed from master.
|
@SimenB cool. Yeah grafana needs that |
@SimenB I think you can also make this |
What does Building grafana with this diff from #11991 now diff --git i/Formula/grafana.rb w/Formula/grafana.rb
index 49b03993f..0e15a2d14 100644
--- i/Formula/grafana.rb
+++ w/Formula/grafana.rb
@@ -27,7 +27,10 @@ class Grafana < Formula
system "go", "run", "build.go", "build"
system "yarn", "install"
system "npm", "install", "grunt-cli", *Language::Node.local_npm_install_args
- system "node_modules/grunt-cli/bin/grunt", "build"
+ args = ["build"]
+ # Avoid PhantomJS error "unrecognized selector sent to instance"
+ args << "--force" unless build.bottle?
+ system "node_modules/grunt-cli/bin/grunt", *args
bin.install "bin/grafana-cli"
bin.install "bin/grafana-server" |
It's a replacement for the bottle block, which you'll delete as well. The users will |
Grafana passed with |
Great! Do you mind opening an issue in the yarn issue tracker about the script issue with BSD userland? |
@@ -1,26 +1,19 @@ | |||
require "language/node" | |||
|
|||
class Yarn < Formula | |||
desc "JavaScript package manager" | |||
homepage "https://yarnpkg.com/" | |||
url "https://yarnpkg.com/downloads/0.21.3/yarn-v0.21.3.tar.gz" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since it's bottle :unneeded
now, please also add
mirror "https://github.com/yarnpkg/yarn/releases/download/v0.21.3/yarn-v0.21.3.tar.gz"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does mirror
do? The Yarn download URL just redirects to the GitHub one (we use the yarnpkg.com URL so we're not tightly-coupled with Github's release system) so it's technically not a mirror, it's exactly the same 😛
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Plot foiled! In that case it would only help if yarnpkg.com went down.
I guess we could use http://distfiles.gentoo.org/distfiles/yarn-v0.21.3.tar.gz
or be mirror-less.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could keep the mirror just in case yarnpkg.com
goes down, but the Windows Chocolatey package also uses yarnpkg.com and I haven't seen any reports of being unable to hit yarnpkg.com.
@Daniel15 any changes you'd like to see here before we 🚢? |
@ilovezfs It seems reasonable to me, but as I mentioned earlier I'm not very familiar with Homebrew (I used it back when I had a Mac, but I switched back to a PC around a year ago). By the way, do you really love ZFS? 😄 |
Heresy!
|
@Daniel15 @SimenB I'm not happy with the current confidence level of "I doubt we'll add additional executables other than yarn and yarnpkg so I think it's fine to just hard-code those two." personally. What will happen for updates of Yarn is that someone will submit a PR to bump to the next version whenever it comes out. If Yarn ever does include additional executables, manpages, etc. then someone will need to notice and update the formula and until then Yarn and Homebrew both end up with a substandard situation. This is why for similar e.g. C/C++ projects we strongly recommend having and using a Thoughts? |
@MikeMcQuaid There are plans to automate PRs to brew with every release of yarn: yarnpkg/yarn#2841. I would think that if other binaries are added, and the Debian script needs to be updated, someone™️ would remember to also make sure the linked binaries are correct. I did have a solution that checked the |
I also just saw the Debian script. I think ideally there'd be a Unix install script (which the Debian one could call, even) which installed to a given
Yeh, I appreciate you investigating that 🙇. |
@Daniel15 that sure would be nice! 😇 |
@@ -1,26 +1,19 @@ | |||
require "language/node" | |||
|
|||
class Yarn < Formula | |||
desc "JavaScript package manager" | |||
homepage "https://yarnpkg.com/" | |||
url "https://yarnpkg.com/downloads/0.21.3/yarn-v0.21.3.tar.gz" | |||
sha256 "0946a4d1abc106c131b700cc31e5c3aa5f2205eb3bb9d17411f8115354a97d5d" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@SimenB let's add
revision 1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. What about mirror
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can go without for now since it's unlikely yarnpkg.com won't resolve
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You'll need to move revision
above head
or it won't pass brew audit --strict
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm currently getting this:
$ brew audit --strict yarn
Error: Error running `rubocop --format json --force-exclusion --config /usr/local/Homebrew/Library/.rubocop.yml /usr/local/Homebrew/Library/Taps/homebrew/homebrew-core/Formula/yarn.rb`
What am I doing wrong?
I did move it though, hopefully no other errors 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can probably fix that by blowing away your ~/.gem
directory
brew install --build-from-source <formula>
, where<formula>
is the name of the formula you're submitting?brew audit --strict <formula>
(after doingbrew install <formula>
)?This doesn't pass it's test, and I'm unable to use it locally after install. There seems to be some issue with the symlink. Instead of following it it's just prepended to the current path. So if I try to run yarn from a deep path:
Why does this happen?
brew test
fails because of this. Might be that the old npm solution put up some extra magic symlinks, but I can't find them, beyond the fact that oldlibexec/bin
also was a symlink.(this is using the old install)
/cc @Daniel15