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

Branch/tag resolution for git dependencies #3720

Closed
ouranos opened this issue Jun 26, 2017 · 11 comments
Closed

Branch/tag resolution for git dependencies #3720

ouranos opened this issue Jun 26, 2017 · 11 comments

Comments

@ouranos
Copy link

ouranos commented Jun 26, 2017

Do you want to request a feature or report a bug?

a bug

What is the current behavior?

We have a repository where we have a branch for the minor version (eg 1.1) and then tag releases for patch versions in this branch (v1.1.0, v1.1.1, ...)

I want to point a project the branch so I can have changes that are not released yet, if I put:

{
  "mno-enterprise-angular": "git+https://git@github.com/<org>/<project>.git#1.1"
}

It actually resolves to the last patch release (eg: v1.1.1) instead of the tip of the branch.

If the current behavior is a bug, please provide the steps to reproduce.

See above, an example is:

{
  "package": "git+https://git@github.com/maestrano/mno-enterprise-angular.git#1.1"
}

The resulting yarn.lock is:

"mno-enterprise-angular@git+https://git@github.com/maestrano/mno-enterprise-angular.git#1.1":
  version "1.1.0"
  resolved "git+https://git@github.com/maestrano/mno-enterprise-angular.git#4fd4c179f5ca315bcf32e25c2cdbfe85102a0ae1"

4fd4c17 is the v1.1.0 tag

What is the expected behavior?

It is expected to resolve to the tip of the branch.
At the moment, I specify the commit in the package.json as a workaround but I can't use yarn upgrade.

Please mention your node.js, yarn and operating system version.

  • yarn v0.24.6
  • node v6.11.0
  • ubuntu 14.04
@bestander
Copy link
Member

@sheerun wanted to address a similar issue #3624 (comment)

@bestander
Copy link
Member

The feature of yarn.lock is that it guarantees repeatable builds.
I think yarn.lock should "freeze" the branch/tag to a specific commit when the lockfile is generated.

@ouranos
Copy link
Author

ouranos commented Jun 26, 2017

Thanks @bestander.

In this case, we'd still freeze the branch to its current commit.
When doing yarn upgrade, it'd update to the new commit (if it has changed).
The problem is that it currently resolves #1.1 as a tag/version instead of a branch.

@bestander
Copy link
Member

I see, there is still confusion, I think we need to have more fields tracking git dependencies: branch, tag, commit hash, tarball hash (if using offline mirror).

Regarding this bug, help from community members is welcome

@Volune
Copy link
Contributor

Volune commented Jun 27, 2017

Is there any documentation or specification about the algorithms in src/util/git.js?
The algorithm that resolves the commit hash from the git url is there, and seems to be confusing. And it was already there in the "first commit", so the git history is not helping.

If I understand well, the git-url-hash, if not a commit-hash, is first used as "SemVer" version constraints, then fallback as a branch/tag name.
I don't think that's an intuitive behavior, because in this case 1.1 is an exact match for the branch name, but v1.1.1 is preferred.

I suggest that we first try to match the git-url-hash against the tags and branches.
If someone needs to force the use of "SemVer" version constraints, he can add a leading space, eg: # 1.1. This should not match any branch/tags, as spaces are not valid in branch/tag names.

@bestander
Copy link
Member

@Volune, there is no documentation, the code evolved trying to satisfy the use cases that Yarn faced, we call this "self documenting code" :)
The git resolution logic is covered with some tests here https://github.com/yarnpkg/yarn/blob/master/__tests__/package-resolver.js.

Feel free to explore this code, we can afford making a breaking change here if it is justified.

@bestander
Copy link
Member

So what priority of these references would you set up:

(none) master => #tag => #branch => #git-hash?
That is at resolution time.

What hash should go to the lockfile?

@ouranos
Copy link
Author

ouranos commented Jun 28, 2017

I think we should still keep the git sha1 in the lockfile. This way we keep the deterministic property when using branches.
If matching on tags, we could use the full tag as it's supposed to be an immutable reference.

For the resolution priority, I'd suggest as @Volune:
(none) master => commit-hash => tag/branch (exact match) => SemVer

@Volune
Copy link
Contributor

Volune commented Jun 28, 2017

I tested a little on NPM5, it doesn't seem to use the "SemVer" algorithm on tags.

I think we should always keep the git sha1 in the lockfile (or the tarball hash, I saw another discussion about that). The tag is only supposed to be immutable, it can still be messed with. In my opinion, using sometimes the sha1 sometimes the full tag would be both more complex and less secure.

Also I've seen some repositories not using master as their default branch. Not having a version in the git url should use that default branch. I think we can get the default branch with git ls-remote --symref <remote> HEAD.


No version git+proto://host/path/repo.git

  • Try to use default branch
  • Try to use master
  • Error

With version git+proto://host/path/repo.git#version

  • Try to use version as a git sha1
  • Try to use version as a tag name
  • Try to use version as a branch name
  • Try to use version as SemVer against existing tags
  • Try to use version as SemVer against existing branches -- (tests branches and tags at same time?)
  • Error

@bestander
Copy link
Member

Sounds good, @Volune.
When the PR is coming? :)
Any idea how to cover it with tests so that we don't regress?

@Volune
Copy link
Contributor

Volune commented Jul 1, 2017

When the PR is coming? :)

Working on it. Looking at the code made me realise there are a lot of things unclear regarding git dependencies, like some optimisations are not shared between git resolvers and git fetchers, or that dependencies using "github:", "bitbucket:" ... prefixes will not have their prepare script executed.
I try to focus on the branch/tag resolution for the moment.

BYK pushed a commit that referenced this issue Jul 26, 2017
**Summary**

For git dependencies, the branch/tag resolution does not always work as expected.
Refactor the code to make the resolution algorithm more explicit and follow discussions in #3720.

**Test plan**

New test in `__tests__/util/git-ref-resolver.js`.

Also, benefit from the fact that Travis CI ran the test with an older Git version. I don't know if we have a minimum git version requirement, it seems we currently don't test against any specific version.

**Implementation details**

I refactored `parseRefs` to return the full ref name, so we can differentiate branches and tags.

I use `git ls-remote --symref` to get the default branch name. This works only in recent versions of git (thanks Travis CI for reporting the issue), so I fall back to another algorithm if `--symref` is unavailable.

I tried not to use the variable name `hash`, which is confusing between the git-url hash and the commit hash.
@BYK BYK closed this as completed Sep 12, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants
@ouranos @BYK @Volune @bestander and others