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

fix(nextjs): CLI binary not found on Windows #6015

Conversation

timfish
Copy link
Collaborator

@timfish timfish commented Oct 23, 2022

Fixes #4829

This was caused by two issues:

Since the CLI getPath() function returns the correct binary location now, and after the above issue is fixed, this PR seems like a sensible route.

@timfish timfish force-pushed the fix/sentry-cli-binary-not-found-windows branch 2 times, most recently from 1cad78e to 8631f12 Compare October 23, 2022 19:29
@timfish timfish force-pushed the fix/sentry-cli-binary-not-found-windows branch from 3f05bf7 to 538e087 Compare October 23, 2022 19:44
@timfish
Copy link
Collaborator Author

timfish commented Oct 23, 2022

Why doesn't @sentry/cli also download the binary if it's missing when called via JavaScript?

All the access seems to go through the async execute function. Why not also check the presence of the binary there and download it if it's missing?

@timfish
Copy link
Collaborator Author

timfish commented Oct 24, 2022

The failures are rather strange:
Unable to resolve path to module '@sentry/nextjs'

Maybe caused by using a non-direct dependency (ie. @sentry/cli).

Perhaps SentryWebpackPlugin should have a static function on it that gets the binary path from the CLI?

This would help with the fact that the @sentry/cli types only seem to work correctly with esModuleInterop enabled.

@lforst
Copy link
Member

lforst commented Oct 24, 2022

Why doesn't @sentry/cli also download the binary if it's missing when called via JavaScript?

All the access seems to go through the async execute function. Why not also check the presence of the binary there and download it if it's missing?

Sadly don't have the answer to those questions. @kamilogorek have you ever thought about doing this?

@lforst
Copy link
Member

lforst commented Oct 24, 2022

As for the errors: You may need to add @sentry/cli to the rollup config's external field, as it is not listed in the package.json's dependencies. I think rollup attempts to bundle/vendor the @sentry/cli and stuff gets weird.

I think it would go here:

packageSpecificConfig: { external: ['next/router'] },

@timfish
Copy link
Collaborator Author

timfish commented Oct 24, 2022

You may need to add @sentry/cli to the rollup config's external field,

Thanks @lforst, that appears to have fixed it!

@lforst
Copy link
Member

lforst commented Oct 25, 2022

The one thing we need to check here is if @vercel/nft will still not include the binary. We had a bunch of problems where users reported they couldn't deploy their projects to lambdas/vercel because the Sentry CLI binary was included in the build and it has something like 15MB pushing them over the 50MB limit.

It was mainly this issue: #3865

@timfish
Copy link
Collaborator Author

timfish commented Oct 25, 2022

The CLI uses this code (and relevant PR) for the binary path, there will be no path to auto resolve:

/**
 * Absolute path to the sentry-cli binary (platform dependent).
 * @type {string}
 */
let binaryPath = eval(
  "require('path').resolve(__dirname, require('os').platform() === 'win32' ? '../sentry-cli.exe' : '../sentry-cli')"
);

Further context:

There are alternatives to eval since @vercel/nft is fooled by basic obfuscation like:

function getPath(): string {
  const parts = [];
  parts.push(__dirname);
  parts.push('..');
  parts.push(`sentry-cli${process.platform === 'win32' ? '.exe' : ''}`);
  return path.resolve(...parts);
}

const binaryPath = getPath();

@timfish
Copy link
Collaborator Author

timfish commented Oct 25, 2022

To avoid Rollup warnings about eval my preference to this PR is to:

and then simply use SentryWebpackPlugin.cliBinaryExists() to detect if the binary exists.

@timfish timfish self-assigned this Oct 25, 2022
@lforst
Copy link
Member

lforst commented Oct 27, 2022

I like that plan! Lemme go through it all. @lobsterkatie do you have any thoughts on this going forward?

@lobsterkatie
Copy link
Member

To avoid Rollup warnings about eval my preference to this PR is to:

and then simply use SentryWebpackPlugin.cliBinaryExists() to detect if the binary exists.

I think we need to add:

But otherwise that sounds like a totally reasonable plan to me.

@timfish
Copy link
Collaborator Author

timfish commented Oct 31, 2022

This PR is superseded by #6096 which uses the route described in the above comments.

@timfish timfish closed this Oct 31, 2022
@timfish timfish deleted the fix/sentry-cli-binary-not-found-windows branch November 29, 2022 09:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

NextJS package does not upload source-maps on WINDOWS
3 participants