Only use npm_package_json when we know it's correct

[appsforartists]

IDK whether you'd prefer to skip npm_package_json in yarn, or to only use it in npm.

yarn runs wireit from the package root, which means npm_package_json will always be /package.json.

Fixes #960

[arcanis]

To be clear, that's only when -T is specified. To quote the documentation, -T "checks the root workspace for scripts and/or binaries instead of the current one", hence why npm_package_json becomes the root one.

[appsforartists]

If I recall correctly, the -T got added because yarn run wireit wasn't finding the root wireit when in a workspace.

So from the POV of wireit, it will always been the root package (since wireit is meant to be a devDependency of the whole project).

[appsforartists]

If I recall correctly, the -T got added because yarn run wireit wasn't finding the root wireit when in a workspace.

So from the POV of wireit, it will always been the root package (since wireit is meant to be a devDependency of the whole project).

[arcanis]

Right, I just don't want someone external reading this comment to think "Wait what; Yarn is broken if it doesn't run run properly" or something of the kind 🙂

[appsforartists]

Just saw uploads large tarball in multiple chunks failed on ubuntu - is that test potentially flaky?

My last change was just updating the comment after arcanis's note - is there a way to check that this PR was fine before that push?

[rictic]

Yeah, I think that test is flaky, re-running it.

[rictic]

This looks good, but please add a test that fails without it. I bet this will be pretty straightforward, check out src/test/basic.test.ts

[appsforartists]

Thanks @rictic!

That was the intent of #963, but I don't know the testing harness well enough to make progress there.

In the mean time, I can add a variant of

test(
  'finds package directory without npm_package_json',
  rigTest(async ({rig}) => {
    // This confirms that we can walk up the filesystem to find the nearest
    // package.json when the npm_package_json environment variable isn't set.
    // This variable isn't set by yarn, pnpm, and older versions of npm.
    const cmdA = await rig.newCommand();
    await rig.write({
      'package.json': {
        scripts: {
          a: 'wireit',
        },
        wireit: {
          a: {
            command: cmdA.command,
          },
        },
      },
    });
    await rig.mkdir('foo/bar/baz');
    const exec = rig.exec(
      IS_WINDOWS
        ? '..\\..\\..\\node_modules\\.bin\\wireit.cmd'
        : '../../../node_modules/.bin/wireit',
      {
        cwd: 'foo/bar/baz',
        env: {
          npm_lifecycle_event: 'a',
        },
      },
    );
    (await cmdA.nextInvocation()).exit(0);
    const res = await exec.exit;
    assert.equal(res.code, 0);
    assert.equal(cmdA.numInvocations, 1);
  }),
);

to basic that tells wireit it's in yarn, lies about cwd/npm_package_json, and checks which it chooses.

[appsforartists]

@rictic @aomarks I took a stab at adding a test for it, adapting the reproduction I left in yarnpkg/berry#5925.

I couldn't get the test to run correctly.

If you try to run yarn inside test, it will see the package-lock.json higher in the tree and panic that it's sharing a repo with npm. You need to seed a yarn.lock file, which tells yarn that it owns its workspace (and lets it ignore any lock files higher in the tree).

Therefore, I have the rig manually writing a yarn.lock file; however, yarn doesn't seem to be able to update yarn.lock when invoked after that. Even if I just rig.touch('yarn.lock'), yarn's changes never come through (even though stdout looks like it's running correctly).

So something about the test environment seems to prevent yarn from writing to yarn.lock, which prevents the test from running. I've tested this PR locally and it worked back in November, so I'm confident it's a good change (even though I understand the desire to enforce/maintain it with a test).

I'm not sure what to do. #962 and #963 have been open for months, trying to get a relatively minor bugfix landed. The truth is that I don't have the time or the knowledge of the codebase to make progress, so I end up stealing a day here or a day there to try to land these PRs. I don't know when I'll next have space to work on these, but I'd rather not have a bugfix languishing for months waiting for tests I can't write, and I feel guilty when my dayjob project slips because I'm stuck on an open-source PR.

Do either of you have tips on why yarn.lock might be immutable or how to resolve that? Would it be possible to land 80717ee, which was this PR before I tried to add tests?