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

Reroll #1816 into a new PR + fix for test failures #2384

Merged
merged 4 commits into from
Jan 4, 2017

Conversation

chrisirhc
Copy link
Contributor

@chrisirhc chrisirhc commented Jan 4, 2017

Summary

Intended to solve #573 . This is a re-application of the changes in #1816 but with a fix for the tests in 3c55f6b. All work is credited to @f-sign . Only attempting to re-run tests and clean up the PR so it doesn't have duplicate commits from master.

Test plan

Tests pass. Left it up to author of #1816 .

Background

This originated from investigating #1816 (comment) .

@chrisirhc chrisirhc changed the title Reroll #1816 into a new PR Reroll #1816 into a new PR + fix for test failures Jan 4, 2017
@gaving
Copy link

gaving commented Jan 4, 2017

Not sure if it's related to the tests or not, but I've the same issue as this after running npm install https://github.com/yarnpkg/yarn#pull/2384/head -g

@bestander
Copy link
Member

@gaving, that is a neat way to install yarn from a PR!

Does the issue with ../lib-legacy/constants reproduce only when you install thisPR?
I doubt it is related to the PR.
It looks more like the .roadrunner cache got invalid and does not clear, try running yarn cache clean

@bestander bestander merged commit 4fc2fe3 into yarnpkg:master Jan 4, 2017
@bestander
Copy link
Member

Thanks @chrisirhc and @f-sign, I merged without squashing

@gaving
Copy link

gaving commented Jan 4, 2017

@bestander Ahh good point, tried with another PR and master even and got the same error.

Sorry for the noise, looking forward to a release!

@sudowork
Copy link

sudowork commented Jan 7, 2017

I'm still running into another issue even after building from master or using the latest nightly build (v0.19.0-20170106.1725):

$ yarn install
yarn install v0.19.0-20170106.1725
info No lockfile found.
[1/4] 🔍  Resolving packages...
error Command failed.
Exit code: 128
Command: git
Arguments: clone ssh://[email protected]:githubuser/repo.git /Users/username/Library/Caches/Yarn/.tmp/bff4d24c292f2c5f1a876330de56ef81
Directory: /Users/username/project
Output:
Cloning into '/Users/username/Library/Caches/Yarn/.tmp/bff4d24c292f2c5f1a876330de56ef81'...
ssh: Could not resolve hostname github.com:githubuser: nodename nor servname provided, or not known
fatal: Could not read from remote repository.

Please make sure you have the correct access rights
and the repository exists.
info Visit https://yarnpkg.com/en/docs/cli/install for documentation about this command.

$ git clone [email protected]:githubuser/repo.git # Just a sanity check
Cloning into 'repo'...
remote: Counting objects: 35, done.
remote: Total 35 (delta 0), reused 0 (delta 0), pack-reused 35
Receiving objects: 100% (35/35), 9.79 KiB | 0 bytes/s, done.
Resolving deltas: 100% (10/10), done.

It appears to be trying to incorrectly clone with the ssh:// prefix.

Doing some minimal debugging so far, it looks like it's going through the GitResolver.

For completeness, here's the dependency string in my package.json: git+ssh://[email protected]:githubuser/repo.git.

@gaving
Copy link

gaving commented Jan 9, 2017

Replicating @sudowork's behaviour after a npm install [email protected].

@gaving
Copy link

gaving commented Jan 9, 2017

Actually after revisiting this comment, things now appear to work for me (didn't before).

Trick is to change URL format:-

git+ssh://git@host:org/repo.git

to

git+ssh://git@host/org/repo.git

Not sure if the semicolon version is supposed to work after this PR, but I'm just happy something is working now. 😄

Full example:-

root@7cf94c933636:/var/www/test# cat package.json
{
  "dependencies": {
    "test": "git+ssh://[email protected]/test/client-node.git"
  }
}
root@7cf94c933636:/var/www/test# yarn install
warning No license field
yarn install v0.19.0
info No lockfile found.
[1/4] Resolving packages...
[2/4] Fetching packages...
[3/4] Linking dependencies...
[4/4] Building fresh packages...
success Saved lockfile.
Done in 22.94s.
root@7cf94c933636:/var/www/test# git clone git+ssh://[email protected]/test/client-node.git test
Cloning into 'test'...
Please disconnect now if you are not authorised.
remote: Counting objects: 37, done.
remote: Compressing objects: 100% (34/34), done.
remote: Total 37 (delta 15), reused 0 (delta 0)
Receiving objects: 100% (37/37), 6.16 KiB | 0 bytes/s, done.
Resolving deltas: 100% (15/15), done.
Checking connectivity... done.

@sudowork
Copy link

sudowork commented Jan 13, 2017

@gaving, Based on the tests, I believe that using the colon separator is supposed to be working. 477c612#diff-00109fff57c65d5eba87e219e6d5a825R44

I may look into why it's not a bit more over the weekend.

balthazar pushed a commit to balthazar/yarn that referenced this pull request Jan 20, 2017
balthazar pushed a commit to balthazar/yarn that referenced this pull request Jan 26, 2017
balthazar pushed a commit to balthazar/yarn that referenced this pull request Jan 26, 2017
bestander pushed a commit that referenced this pull request Feb 5, 2017
* Fix private urls using colon separator

Closes #573, closes #2416.

Related to #2384, #573.

* Remove unused suppression

* Move to dedicated method & add tests
mnsn pushed a commit to mnsn/yarn that referenced this pull request Feb 15, 2017
* Fix private urls using colon separator

Closes yarnpkg#573, closes yarnpkg#2416.

Related to yarnpkg#2384, yarnpkg#573.

* Remove unused suppression

* Move to dedicated method & add tests
@thepian
Copy link

thepian commented Aug 1, 2017

So how are you supposed to use yarn with SSH, it seems clear as mud and if possible more hacky than with npm?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants