Skip to content

(@aws-cdk/aws-lambda-nodejs): Does not use closest/first lockfile. #15847

Closed
@yo1dog

Description

@yo1dog
Contributor

NodejsFunction bundling does not use the correct lockfile if a lockfile for a different package manger exists in a parent directory.

const lockFile = findUp(PackageManager.PNPM.lockFile)
?? findUp(PackageManager.YARN.lockFile)
?? findUp(PackageManager.NPM.lockFile);

This code searches the current and all parent directories for PNPM, then yarn, then NPM lock files. So even if an NPM lockfile exists in the current directory, if a PNPM or yarn lock file exists in any parent directory, CDK will use the PNPM or yarn lockfile. Meaning, if cwd was /fu/bar/baz/, /fu/yarn.lock would be used instead of /fu/bar/baz/package-lock.json.

Reproduction Steps

/home/mike/my-cdk/cdkApp.ts
/home/mike/my-cdk/package-lock.json
/home/mike/yarn.lock

Assuming process.cwd() === '/home/mike/my-cdk'

What did you expect to happen?

/home/mike/my-cdk/package-lock.json is selected as the lockfile. Bundles successfully.

What actually happened?

/home/mike/yarn.lock is selected as the lockfile. Bundling fails.

Even with the --verbose flag, the logs only provide this cryptic message: bash: yarn: command not found. In an attempt to fix, I tried installing yarn. Then the error became: error Couldn't find a package.json file in "/home/mike". At the time, I did not realize there was an errant yarn.lock file in my home directory, so this message was confusing as well.

Environment

  • CDK CLI Version : 1.116.0
  • Framework Version: 1.116.0
  • Node.js Version: v14.17.3
  • OS: Ubuntu 18.04.3 LTS
  • Language (Version): TypeScript (4.3.5)

Other

Workaround is to simply provide the depsLockFilePath param to the NodejsFunction constructor. However, as it stands, it seems one should always include this param as an unrelated lockfile outside the project could break cdk synth.

Instead of walking up directories 3 times looking for the 3 lockfiles, instead walk up once and look for all 3. This would correct the logic to use the first/closest lockfile. Something like:

const lockFile = findUp([
  PackageManager.PNPM.lockFile,
  PackageManager.YARN.lockFile,
  PackageManager.NPM.lockFile
]);

// ...

/**
 * Find a file by walking up parent directories
 */
export function findUp(names: string[], directory: string = process.cwd()): string | undefined {
  const absoluteDirectory = path.resolve(directory);

  for (const name of names) {
    const file = path.join(directory, name);
    if (fs.existsSync(file)) {
      return file;
    }
  }

  const { root } = path.parse(absoluteDirectory);
  if (absoluteDirectory === root) {
    return undefined;
  }

  return findUp(names, path.dirname(absoluteDirectory));
}

Also, enabling more diagnostic messaging around bundling with the verbose flag would be helpful. If the project root, lockfile path, etc. used were reported I would have been able to immediately identify the problem. For example, logging this.packageManager in Bundling.prototype.tryBundle.


This is 🐛 Bug Report

Activity

added
bugThis issue is a bug.
needs-triageThis issue or PR still needs to be triaged.
on Aug 1, 2021
yo1dog

yo1dog commented on Aug 1, 2021

@yo1dog
ContributorAuthor

Sorry submitted too soon by accident. It is done now.

changed the title [-]@aws-cdk/aws-lambda-nodejs: Does not find correct lockfile.[/-] [+]@aws-cdk/aws-lambda-nodejs: Does not use closest/first lockfile.[/+] on Aug 1, 2021
changed the title [-]@aws-cdk/aws-lambda-nodejs: Does not use closest/first lockfile.[/-] [+](@aws-cdk/aws-lambda-nodejs): Does not use closest/first lockfile.[/+] on Aug 1, 2021
nija-at

nija-at commented on Aug 23, 2021

@nija-at
Contributor

The correct solution here would be to stop when the first lock file is found. And if two lock files are found in the same (lowest) directory, it should error and expect depsLockFilePath to be supplied.

Since we have a workaround for this, marking it as a p2.

added
effort/smallSmall work item – less than a day of effort
and removed
needs-triageThis issue or PR still needs to be triaged.
on Aug 23, 2021
bedaka

bedaka commented on Sep 22, 2021

@bedaka

This just took me a lot of time to debug. A random yarn.lock file that was located in a parent folder some levels above my project caused my build to fail. If there is a lock file on project level it really should be used before anything else.

kamzil

kamzil commented on Sep 23, 2021

@kamzil

Had the same experience as @bedaka. To me this feels also like a potential security issue if CDK ends up using some random lock file from outside project folder.

added a commit that references this issue on Sep 23, 2021
c017ed1
removed their assignment
on Nov 25, 2021
added a commit that references this issue on Dec 14, 2021
c4ecd96
github-actions

github-actions commented on Dec 14, 2021

@github-actions
Contributor

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.

added a commit that references this issue on Feb 21, 2022
32a2a92
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

      Development

      Participants

      @kamzil@yo1dog@nija-at@bedaka

      Issue actions

        (@aws-cdk/aws-lambda-nodejs): Does not use closest/first lockfile. · Issue #15847 · aws/aws-cdk