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: re-install lmdb when detecting the absence of @LMDB prebuilt packages #38691

Merged
merged 14 commits into from
Nov 21, 2023

Conversation

JGAntunes
Copy link
Contributor

@JGAntunes JGAntunes commented Nov 6, 2023

Description

Add the ability for gatsby to self heal on the absence of prebuilt lmdb binaries we can use to bundle in lambdas. This is particularly useful to avoid runtime issues with wrong binaries built from source.

We've seen that multiple project structures can lead to this, from particular outdated lockfiles, to npmrc settings instructing the package manager to skip optional package installs.

Documentation

NA

Tests

I've added a set of integration tests which try to first reproduce the issue and then validate that Gatsby was able to self heal and generate the right set of assets for the bundle.

Related Issues

FRA-11

@JGAntunes JGAntunes requested a review from pieh November 6, 2023 19:48
@gatsbot gatsbot bot added the status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer label Nov 6, 2023
@pieh pieh added topic: render-mode Related to Gatsby's different rendering modes and removed status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer labels Nov 7, 2023
// Detect if the prebuilt binaries for lmdb have been installed. These are installed under @lmdb and are tied to each platform/arch. We've seen instances where regular installations lack these modules because of a broken lockfile or skipping optional dependencies installs
function lmdbPrebuiltPackagePresent(): boolean {
try {
require.resolve(`@lmdb/lmdb-${process.platform}-${process.arch}`)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In some cases this would not resolve even if it's actually installed when we try to require(.resolve) from gatsby package context.

Yarn PnP wouldn't allow that because those prebuilt packages are not dependencies of gatsby and are transitive dependency and you can't require/import transitive deps that you package doesn't declare as deps.

In node_modules cases it also is possible this won't work if package manager would decide to structure things like this:

node_modules
├── gatsby
└── lmdb
    └── @lmdb/lmdb-${process.platform}-${process.arch}

this would also mean it doesn't resolve.

For this check to work without relying on node_modules hierarchy we need to resolve prebuilt package from lmdb package context:

Suggested change
require.resolve(`@lmdb/lmdb-${process.platform}-${process.arch}`)
const lmdbRequire = mod.createRequire(
require.resolve(`lmdb`)
)
lmdbRequire.resolve(`@lmdb/lmdb-${process.platform}-${process.arch}`)

The other part is that I'm not sure about is if there are prebuilts for all the platform+arch combinations that exist. If prebuilt for the combo doesn't exist we will always try to install lmdb but that wouldn't help, so maybe before we even attempt to install - we should try to check if @lmdb/lmdb-${process.platform}-${process.arch} is included in list of available prebuilts (gathered from lmdb's optional dependencies list):

[
  '@lmdb/lmdb-darwin-arm64', 
  '@lmdb/lmdb-darwin-x64', 
  '@lmdb/lmdb-linux-arm', 
  '@lmdb/lmdb-linux-arm64', 
  '@lmdb/lmdb-linux-x64', 
  '@lmdb/lmdb-win32-x64'
]

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Greate suggestions, I'll add those in 👍

@@ -35,6 +37,71 @@ function getApisToRemoveForQueryEngine(): Array<GatsbyNodeAPI> {
return apisToRemove
}

const getInternalPackagesCacheDir = (): string =>
path.join(process.cwd(), `.cache/internal-packages`)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do wonder if there is something we could do to "sandbox" this to not have potential repo/site level .npmrc (or even user level .npmrc locally) potentially cause problems with installing optional packages. If there is .npmrc with omit=optional (or however the syntax it is) - that would use in this directory as well?

Alternatively, maybe instead of installing whole lmdb (and let that install optional packages with prebuilds) - maybe we try to install those prebuild package directly? Maybe then it would be less prone to inherit same problem that cause those not to be installed normally in the first place?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alternatively, maybe instead of installing whole lmdb (and let that install optional packages with prebuilds) - maybe we try to install those prebuild package directly?

I guess what I wonder is, in the presence of @lmdb/x package will that always be favoured even if there's a build from source in the lmdb directory? (I can dig through the code to try to understand that but tbh I worry that we might miss something by doing so)

Copy link
Contributor

@pieh pieh Nov 9, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess what I wonder is, in the presence of @lmdb/x package will that always be favoured even if there's a build from source in the lmdb directory? (I can dig through the code to try to understand that but tbh I worry that we might miss something by doing so)

It looks like general order of loading is to first try to load from build/Release first before trying optional packages. However this relies on fact that there is install script that before trying to compile from source check if there is binary that can be loaded - and when prebuilts are installed - it finds those and that check prevent from even compiling from source ( https://unpkg.com/browse/node-gyp-build-optional-packages/ is doing all those things ).

However - our lmdb-bundling-patch replace runtime usage of node-gyp-build(-optional-packages) from lmdb and replace that with direct path to .node file, so we are not subject to regular behavior when bundling for lambdas as we get rid of it here:

.replace(
`require$1('node-gyp-build-optional-packages')(dirName)`,
`require(${JSON.stringify(lmdbBinaryLocation)})`
)
.replace(
`require$1('node-gyp-build')(dirName)`,
`require(${JSON.stringify(lmdbBinaryLocation)})`
)
.replace(
`loadNAPI__default["default"](dirName);`,
`require(${JSON.stringify(lmdbBinaryLocation)})`
)

So I think if we can install just one of prebuild packages like @lmdb/lmdb-linux-x64 and get path to .node file and make the lmdb bundling patch use that - I think this should be fine (?)

Also when we install whole lmdb in .cache/internal-packages, we don't actually use lmdb instance from there currently - we could add webpack alias to use that instance of lmdb package for lambda, but my biggest hang up with it is that installing main lmdb package it in nested directory could inherit any configuration that caused those prebuilt packages to not be installed in first place.

PS. after going through node-gyp-build-optional-packages I also found that:

build-from-source = true

in .npmrc would also likely cause the issue (similarly to omit = optional)

@JGAntunes JGAntunes self-assigned this Nov 9, 2023
@JGAntunes
Copy link
Contributor Author

@pieh I've updated the logic to only install the specific@lmdb package and use the force flag to override for the loader to override the binary in use. Let me know what you think. I've tested this out locally in a project with a package built from source and it appears to be working.
Wanted to write some tests for this but I'm wondering what would be the best approach for it? An e2e-test?

@pieh
Copy link
Contributor

pieh commented Nov 10, 2023

Wanted to write some tests for this but I'm wondering what would be the best approach for it? An e2e-test?

That surely would provide the most confidence, but I'm not sure what setup for it would be best - easiest/quickest right now would be to have fixture test site with .npmrc with one of known triggers for the problem and deploy to Netlify and try to hit SSR or DSG route and assert it's not 500 status. But if setup would change in the future on Netlify that test might not be testing the problem anymore.

More standalone e2e setup could use different docker images for build and serve (picked in a way that would trigger the problem - so image for build would have newer GLIBC than image for serve - at least if I understand this part correctly), but this seems quite a bit of work to get that done?

Alternatively we don't do full on actual e2e - instead something more like integration - we still have fixture test site that would build lmdb from source, let it run build and we "just" inspect .node binary in produced SSR/DSG lambda - not sure what would be the best way to assert that correct .node is being bundled for lambda - maybe just file comparison against node_modules/lmdb/build/Release/lmdb.node would be enough (at least assuming that one that is built from source is not exactly like one that is in prebuilds)? With setup right now we could also check the name of binary - if .cache/query-engine/assets/build/Release/lmdb.node is in lambda - then that's prebuilt - but this relies on some details of bundling that could potentially change (unrelated to changes done in this PR), so I don't quite like that kind of assertion.

@pieh
Copy link
Contributor

pieh commented Nov 10, 2023

Let me know what you think.

The code part seems great and I don't have any more notes on it 👍

@JGAntunes JGAntunes marked this pull request as ready for review November 17, 2023 14:43
Copy link
Contributor

@pieh pieh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@pieh pieh added the bot: merge on green Gatsbot will merge these PRs automatically when all tests passes label Nov 21, 2023
@gatsbybot gatsbybot merged commit e3365ab into master Nov 21, 2023
31 of 33 checks passed
@gatsbybot gatsbybot deleted the fix/install-missing-lmdb-prebuilt-packages branch November 21, 2023 14:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bot: merge on green Gatsbot will merge these PRs automatically when all tests passes topic: render-mode Related to Gatsby's different rendering modes
Projects
Status: Published
Development

Successfully merging this pull request may close these issues.

3 participants