test(integration): Adds assertion to exemplify wrong path resolution in lock file #4717

joscha · 2017-10-16T02:40:15Z

The added assertions make sure that the relative path generated in the lock file points to the
installation, not the yarn cache

This exemplifies the problem outlined in #4388:

joscha · 2017-10-16T04:05:04Z

Error seems to be produced here:

yarn/src/config.js

Line 412 in 4c38ca7

return path.join(this.cacheFolder, `${name}-${uid}`);

where the cache path is used as a starting point to generate an absolute path, coming from:

yarn/src/cli/commands/install.js

Line 832 in 4c38ca7

loc = this.config.generateHardModulePath(ref);

and further when the dependency gets resolved initially, where it should have been something more similar to a 'workspace' type instead of a 'copy' type:

yarn/src/resolvers/exotics/file-resolver.js

Line 76 in 4c38ca7

type: 'copy',

BYK · 2017-10-26T10:26:32Z

Hi @joscha! Thanks for the PR! Obviously, we cannot merge this since it breaks CI :)

Are you planning to submit a fix for the problem? If not can we have this test split up and skip the failing part so we can keep the code and the CI?

joscha · 2017-10-26T11:20:47Z

Hi @BYK - I opened the PR to illustrate the problem outlined in #4388 in a test. This is currently blocking us from upgrading from 0.27.5 to 1.x, so I would love to provide a fix. Unfortunately, I dug around in the code and can't perfectly pinpoint where the fix will need to be made, as there are a few places where it could be, but I think something more general needs correction than a simple path fix, hence my comment #4717 (comment) - do you know anyone I could have a short chat with to find out in which module the fix would live?

BYK · 2017-10-31T11:37:39Z

@joscha - sorry for the late responses. Unfortunately, PRs are not the best place to have quick conversations anymore 😄 Come to our Discord Channels anytime and ping me or anyone really to initiate a chat. I feel like this issue is somewhat related to #3453.

I'll do some more digging and come back to you. Thanks a lot for sticking with us although us not responding very quickly :)

BYK · 2017-10-31T17:25:48Z

@joscha I've pushed some changes to your branch and did some hacky things to fix the issue but I think we need a larger refactoring here.

@rally25rs @arcanis @kaylieEB - Please have a look at the new code now. It also fixes #3453. I think this is super-hacky but maybe it is an "OK" start and we can start fixing these intermingled parts?

BYK · 2017-10-31T17:30:46Z

Woo, I've broken so many other tests now :D

rally25rs · 2017-10-31T17:36:19Z

src/fetchers/copy-fetcher.js

@@ -5,11 +5,10 @@ import BaseFetcher from './base-fetcher.js';
 import * as fs from '../util/fs.js';

 export default class CopyFetcher extends BaseFetcher {
-  async _fetch(): Promise<FetchedOverride> {
-    await fs.copy(this.reference, this.dest, this.reporter);


I don't see this copy anywhere else, just removed. Was it not needed? Is something else performing the copy now?

Since "file" dependencies are already on the disk, they are now copied at the linking stage, directly from the source instead of from the cache folder.

rally25rs · 2017-10-31T17:38:57Z

src/config.js

@@ -395,6 +395,10 @@ export default class Config {
      return pkg.location;
    }

+    if (pkg.remote.type === 'copy') {


The couple pkg.remote.type === 'copy' checks feel like an implementation detail that should be left to the remote. (leaky abstraction?) Perhaps this module path creation should be part of the remote and not part of config (config feels like a weird place for it anyway).

Totally agree. I think we need to extend the remote information to allow "directly available" packages. This may become useful for other things in the future but I don't know how directly. Right now it feels specific to link: and file: protocols so I was unsure about extending this data structure or not.

Looks like tarball-resolver can also report a type of copy. Not sure if that is OK or not...

// set remote so it can be "fetched" pkgJson._remote = { type: 'copy', resolved: `${url}#${hash}`, hash, registry, reference: dest, };

rally25rs

Assuming the CI failures are not actually related to this PR, 👍 (edit: looking at the CI results, it's hard to tell what's causing these...)

joscha · 2017-11-06T22:49:46Z

\o/

joscha · 2017-11-13T22:27:41Z

@BYK any chance to get this in?

joscha · 2017-11-24T06:08:35Z

@BYK is the general gist of the changes in this PR what we want to do? If yes, I can try and make tests go green.

BYK · 2017-11-27T11:00:14Z

@joscha I guess so. I feel like this requires a bit more refactoring around how we use cache and assume it is there so not sure if it would be fun times for you to figure that out. That said if you really want to try, who am I to stop an enthusiastic soul? 😁

joscha · 2017-11-27T11:05:02Z

That said if you really want to try, who am I to stop an enthusiastic soul?

😆 haha, hence my question if the general approach is fine, because looking at the code right now, I couldn't definitely say it is. Are there user stories somewhere, e.g. When I have a local file:-dependency, then I expect the files to be copied into the node modules folder and the yarn cache not to be populated, etc.?

BYK · 2017-11-27T11:21:33Z

When I have a local file:-dependency, then I expect the files to be copied into the node modules folder and the yarn cache not to be populated, etc.?

I don't think there's a user scenario but since file: dependencies are local, unless you have used yarn link they should never have been copied to cache in the first place if you ask me. This is what I mean with larger refactoring. Right now, Yarn tries to load everything from the cache folder with the exception of links. That most probably needs to change but it didn't look too easy when I last tried.

joscha · 2017-11-27T11:33:04Z

I suppose it might have been done for multiple runs of `install`, e.g. a local dependency should probably stay stable if you just do another `install` even if the folder of the target dependency has been touched, e.g.: X is v1 Y depends on file:..X with yarb.lock X=1 Install in Y (X in Y/node_modules is at 1) X gets touched and bumped to 2 Install in Y (X in Y/node_modules should stay at 1) Delete Y/node_modules Install in Y (X is at 2, yarn.lock of Y is updated with X=2) Which means that there needs to be a copy of X which isn't touched (unless that one copy is the one in Y/node_modules?) It's hard to say what is right without having a user story reasoning about it.

…

On 27 Nov. 2017 22:21, "Burak Yiğit Kaya" ***@***.***> wrote: When I have a local file:-dependency, then I expect the files to be copied into the node modules folder and the yarn cache not to be populated, etc.? I don't think there's a user scenario but since file: dependencies are local, unless you have used yarn link they should never have been copied to cache in the first place if you ask me. This is what I mean with larger refactoring. Right now, Yarn tries to load everything from the cache folder with the exception of links. That most probably needs to change but it didn't look too easy when I last tried. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#4717 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AALehrRm5uevUsOCnXL6XYZhXBF8xtrJks5s6ptGgaJpZM4P6A_q> .

BYK · 2017-11-27T11:52:35Z

@joscha I feel like it is fine to sync/copy at all times. That would be a divergence from the old npm behavior but would align with the new npm behavior where file: is treated as link: since I think that's what the developers expect anyway. In that case, bypassing the cache makes sense. Even if we want to use the cache, we need to namespace these packages so they don't collide with things from other registries etc.

joscha · 2018-02-27T13:12:34Z

This seems to be fixed in 1.5.1. I propose to get the tests in, to ensure it won't be broken in the future by accident.

arcanis · 2018-02-27T18:00:52Z

@joscha Do you mind rebasing the PR? I'll see to merge it!

…in lock file The added assertions make sure that the relative path generated in the lock file points to the installation, not the yarn cache yarnpkg#4388

joscha · 2018-02-27T21:20:35Z

rebased @arcanis

buildsize · 2018-02-27T21:23:22Z

This change will decrease the build size from 10.49 MB to 10.49 MB, a decrease of 5.33 KB (0%)

File name	Previous Size	New Size	Change
yarn-[version].noarch.rpm	909.34 KB	908.96 KB	-393 bytes (0%)
yarn-[version].js	3.95 MB	3.95 MB	-2.21 KB (0%)
yarn-legacy-[version].js	4.1 MB	4.1 MB	-2.2 KB (0%)
yarn-v[version].tar.gz	914.8 KB	914.26 KB	-552 bytes (0%)
yarn_[version]all.deb	675.21 KB	675.22 KB	12 bytes (0%)

joscha · 2018-02-27T21:27:05Z

ah, I am not sure what I am seeing locally with 1.5.1 - seems this problem still exists in master...

joscha · 2018-02-27T21:58:34Z

Just tested again locally. I am unsure what the hiccup was, but this problem still exists in 1.5.1 unfortunately.

tsdorsey · 2018-03-21T21:19:17Z

Forgive me if this is unrelated but another consideration when tackling this issue is that yarn will convert absolute paths in package.json to relative ones in yarn.lock. In my use case, that breaks things (which I can expound upon if need be but I do not think relavant). I would expect that a local dependency (absolute or relative path) should not be altered by the yarn.lock generation process.

joscha · 2018-06-12T02:39:38Z

@arcanis I fixed the merge commit up, you can now see the problem again.

joscha mentioned this pull request Oct 16, 2017

File dependencies from transitive dependencies are stored incorrectly in lock file #4388

Open

BYK requested review from arcanis, bestander and kaylie-alexa October 31, 2017 17:24

BYK requested review from rally25rs and removed request for bestander October 31, 2017 17:25

BYK self-assigned this Oct 31, 2017

rally25rs reviewed Oct 31, 2017

View reviewed changes

rally25rs approved these changes Nov 6, 2017

View reviewed changes

test(integration): Adds assertion to exemplify wrong path resolution …

c0ec6eb

…in lock file The added assertions make sure that the relative path generated in the lock file points to the installation, not the yarn cache yarnpkg#4388

joscha force-pushed the joscha/relative-paths-cache-dir branch from 854fb1b to c0ec6eb Compare February 27, 2018 21:20

arcanis and others added 2 commits June 8, 2018 23:49

Merge branch 'master' into joscha/relative-paths-cache-dir

aa61ba6

Update integration.js

1e51ee2

sokki mentioned this pull request Feb 18, 2019

Don't write cache path into yarn.lock for nested file dependencies #7042

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test(integration): Adds assertion to exemplify wrong path resolution in lock file #4717

test(integration): Adds assertion to exemplify wrong path resolution in lock file #4717

joscha commented Oct 16, 2017

joscha commented Oct 16, 2017 •

edited

BYK commented Oct 26, 2017

joscha commented Oct 26, 2017

BYK commented Oct 31, 2017

BYK commented Oct 31, 2017

BYK commented Oct 31, 2017

rally25rs Oct 31, 2017

BYK Oct 31, 2017

rally25rs Oct 31, 2017

BYK Oct 31, 2017

rally25rs Oct 31, 2017

rally25rs left a comment •

edited

joscha commented Nov 6, 2017

joscha commented Nov 13, 2017

joscha commented Nov 24, 2017

BYK commented Nov 27, 2017

joscha commented Nov 27, 2017

BYK commented Nov 27, 2017

joscha commented Nov 27, 2017 via email

BYK commented Nov 27, 2017

joscha commented Feb 27, 2018

arcanis commented Feb 27, 2018

joscha commented Feb 27, 2018

buildsize bot commented Feb 27, 2018

joscha commented Feb 27, 2018

joscha commented Feb 27, 2018

tsdorsey commented Mar 21, 2018

joscha commented Jun 12, 2018

test(integration): Adds assertion to exemplify wrong path resolution in lock file #4717

Are you sure you want to change the base?

test(integration): Adds assertion to exemplify wrong path resolution in lock file #4717

Conversation

joscha commented Oct 16, 2017

joscha commented Oct 16, 2017 • edited

BYK commented Oct 26, 2017

joscha commented Oct 26, 2017

BYK commented Oct 31, 2017

BYK commented Oct 31, 2017

BYK commented Oct 31, 2017

rally25rs Oct 31, 2017

Choose a reason for hiding this comment

BYK Oct 31, 2017

Choose a reason for hiding this comment

rally25rs Oct 31, 2017

Choose a reason for hiding this comment

BYK Oct 31, 2017

Choose a reason for hiding this comment

rally25rs Oct 31, 2017

Choose a reason for hiding this comment

rally25rs left a comment • edited

Choose a reason for hiding this comment

joscha commented Nov 6, 2017

joscha commented Nov 13, 2017

joscha commented Nov 24, 2017

BYK commented Nov 27, 2017

joscha commented Nov 27, 2017

BYK commented Nov 27, 2017

joscha commented Nov 27, 2017 via email

BYK commented Nov 27, 2017

joscha commented Feb 27, 2018

arcanis commented Feb 27, 2018

joscha commented Feb 27, 2018

buildsize bot commented Feb 27, 2018

joscha commented Feb 27, 2018

joscha commented Feb 27, 2018

tsdorsey commented Mar 21, 2018

joscha commented Jun 12, 2018

joscha commented Oct 16, 2017 •

edited

rally25rs left a comment •

edited