yarnpkg / yarn

The 1.x line is frozen - features and bugfixes now happen on https://github.com/yarnpkg/berry
https://classic.yarnpkg.com
Other
41.42k stars 2.72k forks source link

Git is required to be installed when git dependencies are resolved via http(s) #7311

Open xPaw opened 5 years ago

xPaw commented 5 years ago

Running yarn global add thelounge@v3.1.0-pre.1 will end up trying to execute the git ls-remote command, which fails when git is not installed.

https://github.com/yarnpkg/yarn/blob/eb2b565bb9b948e87b11119482ebc184a9d66141/src/util/git.js#L165

However I find this behaviour weird:

  1. The package is being referenced as github: so it should resolve into a https url https://github.com/kiwiirc/irc-framework/blob/964b7df705eca5ce192ebadd65cd88145dc481af/package.json#L13
  2. Lock file in thelounge's release already has that dependency resolved into a github codeload https url https://github.com/thelounge/thelounge/blob/edf7f001ae490ddb638dfc0404491fba828e544a/yarn.lock#L7366-L7369

So the questions are:

  1. Why is it not resolving github: into a codeload url during yarn global add or yarn add?
  2. Why is it not using the already resolved url from the yarn.lock which exists in the published package?

This behaviour is inconsistent, I would expect the add commands to resolve into a https url, just like the already exisiting yarn.lock did during a package upgrade.

DanielRuf commented 5 years ago

Hi @xPaw,

In general lockfiles of installed packes are not used when they are installed.

Did you already try kiwiirc/runes instead of github:kiwiirc/runes?

DanielRuf commented 5 years ago

From the npm docs, same for yarn:

npm install github:/[#]:

Install the package at https://github.com/githubname/githubrepo by attempting to clone it using git.

It makes no difference if you clone with https or ssh. Both are using git clone.

xPaw commented 5 years ago

But there's specifically a github resolver in yarn that can return a tarball url, which avoids using git (that url is put in the yarn lock file), why is it not used during global install?

https://github.com/yarnpkg/yarn/blob/eb2b565bb9b948e87b11119482ebc184a9d66141/src/resolvers/exotics/github-resolver.js

DanielRuf commented 5 years ago

global install

There is no lockfile used on this level afaik.

DanielRuf commented 5 years ago

Or are you speaking about a local dependency (yarn add / yarn install)?

xPaw commented 5 years ago

I understood that, but my question is why doesn't the install resolve to using a tarball instead of git?

Basically, thelounge@next can't be installed globally without git because one of the dependencies specifies github:. And it's odd to me because Yarn has a github->tarball resolver (and lockfile has a tarball url, but lock file isnt used as it turns out).

DanielRuf commented 5 years ago

why doesn't the install resolve to using a tarball instead of git?

As described in the docs, if it is github: is uses git clone. The docs of npm are a bit more verbose than the docs of yarn (which was meant as a replacement / better alternative for npm).

xPaw commented 5 years ago

What's this supposed to do then?

https://github.com/yarnpkg/yarn/blob/eb2b565bb9b948e87b11119482ebc184a9d66141/src/resolvers/exotics/hosted-git-resolver.js#L208-L213

DanielRuf commented 5 years ago

The tarball may be only for the cache as the local npm / yarn caches use the tarballs which are then locally extracted into node_modules.

Probably relevant tests: https://github.com/yarnpkg/yarn/blob/eb2b565bb9b948e87b11119482ebc184a9d66141/__tests__/util/git.js#L62-L81

https://github.com/yarnpkg/yarn/blob/eb2b565bb9b948e87b11119482ebc184a9d66141/__tests__/resolvers/exotics/hosted-git-resolver.js

https://github.com/yarnpkg/yarn/blob/eb2b565bb9b948e87b11119482ebc184a9d66141/src/resolvers/exotics/github-resolver.js#L6

https://github.com/yarnpkg/yarn/blob/eb2b565bb9b948e87b11119482ebc184a9d66141/__tests__/package-request.js#L35

And some probably relevant issue: https://github.com/yarnpkg/yarn/issues/4393

I have tried the same and on my side it uses the https endpoint (cleared the Yarn cache before each test).

yarn add thelounge@v3.1.0-pre.1 --registry https://registry.npmjs.org --verbose > debug.log yields in the following:

verbose 1.237 Performing "HEAD" request to "https://github.com/kiwiirc/runes".
verbose 1.239 Request "https://registry.npmjs.org/yarn" finished with status code 200.

yarn add thelounge@next --registry https://registry.npmjs.org --verbose > debug2.log yields in the following:

verbose 3.855 Performing "GET" request to "https://raw.githubusercontent.com/kiwiirc/runes/a8d7f76408f38749cfec9dec7c7a1430300f2645/package.json".
verbose 3.931 Request "https://raw.githubusercontent.com/kiwiirc/runes/a8d7f76408f38749cfec9dec7c7a1430300f2645/package.json" finished with status code 200.
[2/4] Fetching packages...

Can you run the same commands and check the output?

xPaw commented 5 years ago

It does look like it actually downloads from https,

verbose 4.412 Performing "HEAD" request to "https://github.com/kiwiirc/runes".
verbose 5.538 Request "https://github.com/kiwiirc/runes" finished with status code 200.
verbose 6.562 Performing "GET" request to "https://github.com/kiwiirc/runes.git/info/refs?service=git-upload-pack".
verbose 7.263 Request "https://github.com/kiwiirc/runes.git/info/refs?service=git-upload-pack" finished with status code 200.
verbose 8.088 Performing "GET" request to "https://raw.githubusercontent.com/kiwiirc/runes/a8d7f76408f38749cfec9dec7c7a1430300f2645/package.json".
verbose 8.401 Request "https://raw.githubusercontent.com/kiwiirc/runes/a8d7f76408f38749cfec9dec7c7a1430300f2645/package.json" finished with status code 200.

however, it still calls the following git command, which means git must be installed for it to work. I have a console.log in yarn cli whenever it tries to spawn a process:

git [ 'ls-remote',
  '--symref',
  'https://github.com/kiwiirc/runes.git',
  'HEAD' ]

Here's a CI job log where it actually fails: https://travis-ci.com/thelounge/thelounge-docker/jobs/204308778#L1004-L1016

Looks like it's being called by resolveDefaultBranch here: https://github.com/yarnpkg/yarn/blob/eb2b565bb9b948e87b11119482ebc184a9d66141/src/util/git.js#L450

DanielRuf commented 5 years ago

Looks like it's being called by resolveDefaultBranch here:

Good catch. Could be the current implementation to get the default branch from hosted Git repositories (afaik we can not get the default branch from the web interfaces as there is no default URL for the default branch - we might have to check this).

The current assumption is probably that developers also have Git installed. But so far this is likely another issue which is not the same as the initial issue comment and title.

Let's check if we can provide a solution for https (I doubt it but probably there is one). Also see https://stackoverflow.com/a/54204231/753676 and https://stackoverflow.com/a/28669194/753676

So cloning it once seems to be needed.

xPaw commented 5 years ago

Am I missing something? Doesn't it download the HEAD commit from git-upload-pack which is also resolved from https?

To me it looks like if the git call didn't error out due to missing git, it would install just fine?

It requests "https://raw.githubusercontent.com/kiwiirc/runes/a8d7f76408f38749cfec9dec7c7a1430300f2645/package.json" before calling git ls-remote command, so the ref is already resolved at that point.

DanielRuf commented 5 years ago

I am not sure about this. We would have to check this. Maybe something was overseen (the default branch check).

My answer is just a statement about the current behavior (that the check for the default branch seems to be also running in this case). And I do not yet know if it is needed. At least the git-upload-pack may not contain all needed objects like the current branch (I have never directly used this as this is not intended for users).

https://git-scm.com/docs/git-upload-pack

To me it looks like if the git call didn't error out due to missing git, it would install just fine?

Exactly.

Looks like it's being called by resolveDefaultBranch here: A reference can be a commit or a branch (and this is normally set in the HEAD file for the default branch).

https://github.com/yarnpkg/yarn/blob/eb2b565bb9b948e87b11119482ebc184a9d66141/src/util/git/git-ref-resolver.js#L50-L54

Currently we are at a point where it is not about the lockfile anymore. Can you update the issue title and maybe update the initial comment to reflect that some git commands also run when we use the http(s) variant to get the dependency files?