fix(hosted-git-resolver): ensure prepare script is executed for hosted git repo #6131
Conversation
…d git repo Use git-fetcher, not tarball-fetcher to deal with hosted git repo. tarball-fetcher has no knowledge about the difference between normal npm package tarball, and tarball provided by hosted git (which is NOT a proper npm package). Obviously this fix has performance penalty, but it's necessary to support prepare script. Another possible solution is to add git-tarball-resolver (extending tarball-resolver), but I don't understand the code base enough to be able to extract hasPrepareScript and fetchFromInstallAndPack out from git-resolver. closes #5235
|
|
|
||
| // If the url is accessible over git archive then we should immediately delegate to | ||
| // the git resolver. | ||
| // | ||
| // NOTE: Here we use a different url than when we delegate to the git resolver later on. | ||
| // This is because `git archive` requires access over ssh and github only allows that | ||
| // if you have write permissions | ||
| const sshGitUrl = Git.npmUrlToGitUrl(sshUrl); | ||
| if (await Git.hasArchiveCapability(sshGitUrl)) { |
There was a problem hiding this comment.
Note I removed the Git check archive and new Git(...).
GitResolver creates client = new Git(...), and client.init() does the same thing (check archive and resolve committish).
This just removes duplicated effort.
|
I guess I have to wait for someone cleanup test-macos-node10 and appveyor test on current master. |
|
Any updates on where this PR is at? This is functionality we really need as we migrate some old apps to a modern build process with webpack and yarn. |
|
It seems yarn team has not cleaned up CI/test setup on the master yet. I don't think any PR is in their consideration now. If you want a temporary solution, install my branch as your global yarn which I did. Because the repo setup, you cannot install directly from my github repo (it would not build). Do |
|
Should prepare scripts be called for all dependencies (including tarballs)? If so, then perhaps this PR should instead move the logic into |
|
Nope, a tarball can be a valid npm package (like prepared by |
yarn has a bug where it treats github: deps differently from git:// deps (<yarnpkg/yarn#6131>), but it also a separate bug where it just dies if a git:// depenency uses prepare and also depends on the same package as the original package (<yarnpkg/yarn#6312>). Maybe we should just switch to npm...
yarn has a bug where it treats github: deps differently from git:// deps (<yarnpkg/yarn#6131>), but it also a separate bug where it just dies if a git:// depenency uses prepare and also depends on the same package as the original package (<yarnpkg/yarn#6312>). Work around by cloning the ThingTalk repository separately and using yarn link. Maybe we should just switch to npm...
|
@arcanis Could you review this PR which fixes previously-PR-welcomed issue? Sorry if bothering you! |
|
@arcanis is it safe to assume that this PR will never land? (because Yarn 1 is no longer receiving features?) |
|
Closing as fixed in v2 where the |
merceyz
added
the
fixed-in-modern
Summary
Use git-fetcher, not tarball-fetcher to deal with hosted git repo. tarball-fetcher has no knowledge about the difference between normal npm package tarball, and tarball provided by hosted git (which is NOT a proper npm package).
Obviously this fix has performance penalty, but it's necessary to support prepare script.
Another possible solution is to add git-tarball-resolver (extending tarball-resolver), but I don't understand the code base enough to be able to extract hasPrepareScript and fetchFromInstallAndPack out from git-resolver.
closes #5235
Test plan
Tested my apps with github public repo, bitbucket public and private repos, all can now trigger prepare scripts.
Tested
yarn importon my apps.Tested all above with no yarn cache (
yarn cache clean) and full yarn cache.