-
Notifications
You must be signed in to change notification settings - Fork 10.3k
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
gatsbybot
merged 14 commits into
master
from
fix/install-missing-lmdb-prebuilt-packages
Nov 21, 2023
Merged
Changes from all commits
Commits
Show all changes
14 commits
Select commit
Hold shift + click to select a range
67c71f3
fix: re-install lmdb when detecting the absence of @lmdb prebuilt pac…
JGAntunes 9e50274
chore: set the correct root lmdb and add further comments
JGAntunes 619abf1
chore: linter
JGAntunes c3cd62b
chore: linting
JGAntunes 22be691
chore: make the install check for reliable and install @lmdb instead …
JGAntunes 2555ba9
fix: no need to touch the patch, we can leverage the force binary flag
JGAntunes ae45348
chore: add integration test for lmdb fix
JGAntunes 965db45
Merge branch 'master' into fix/install-missing-lmdb-prebuilt-packages
JGAntunes 7c0e053
chore: add CCI config for new integration test
JGAntunes 01bf938
add integration_tests_esm_in_gatsby_files to jobs
pieh 915dbe0
fix: integration test for lmdb-regeneration
JGAntunes f2deaac
fix: cli path in test
JGAntunes 9c23192
chore: actually add lmdb's dist directory
JGAntunes 8d793e4
fix: back to the npmrc and just rm -rf @lmdb modules
JGAntunes File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
__tests__/__debug__ | ||
node_modules | ||
yarn.lock |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
build_from_source=true |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,21 @@ | ||
MIT License | ||
|
||
Copyright (c) 2018 gatsbyjs | ||
|
||
Permission is hereby granted, free of charge, to any person obtaining a copy | ||
of this software and associated documentation files (the "Software"), to deal | ||
in the Software without restriction, including without limitation the rights | ||
to use, copy, modify, merge, publish, distribute, sublicense, and/or sell | ||
copies of the Software, and to permit persons to whom the Software is | ||
furnished to do so, subject to the following conditions: | ||
|
||
The above copyright notice and this permission notice shall be included in all | ||
copies or substantial portions of the Software. | ||
|
||
THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR | ||
IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, | ||
FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE | ||
AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER | ||
LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, | ||
OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE | ||
SOFTWARE. |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
## Artifacts test suite | ||
|
||
This integration test suite helps us assert our mechanism to heal back from a broken lmdb installation is working by validating we install and use the correct prebundled binary for our platform |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,59 @@ | ||
const path = require(`path`) | ||
const execa = require(`execa`) | ||
const mod = require("module") | ||
const fs = require(`fs-extra`) | ||
|
||
jest.setTimeout(100000) | ||
|
||
const rootPath = path.resolve(__dirname, "../") | ||
|
||
describe(`Lmdb regeneration`, () => { | ||
test(`gatbsy build detects lmdb setup built from source and installs pre-buit package`, async () => { | ||
const lmdbNodeModulesPath = path.resolve(rootPath, "node_modules", "lmdb") | ||
// Make sure we clear out the current `@lmdb` optional dependencies | ||
const pathsToRemove = [ | ||
path.resolve(rootPath, "node_modules", "@lmdb"), | ||
path.resolve(rootPath, "node_modules", "gatsby", "node_modules", "@lmdb"), | ||
] | ||
for (let path of pathsToRemove) { | ||
fs.rmSync(path, { force: true, recursive: true }) | ||
} | ||
// Check the lmdb instance we have installed does have a binary built from source since we need it to reproduce the fix we're trying to test | ||
// If this check fails then it means our fixture is wrong and we're relying on an lmdb instance with prebuilt binaries | ||
const builtFromSource = fs.existsSync( | ||
path.resolve(lmdbNodeModulesPath, "build", "Release", "lmdb.node") | ||
) | ||
expect(builtFromSource).toEqual(true) | ||
|
||
const options = { | ||
stderr: `inherit`, | ||
stdout: `inherit`, | ||
cwd: rootPath, | ||
} | ||
const gatsbyBin = path.resolve(rootPath, `node_modules`, `gatsby`, `cli.js`) | ||
await execa(gatsbyBin, [`build`], options) | ||
|
||
// lmdb module with prebuilt binaries for our platform | ||
const lmdbPackage = `@lmdb/lmdb-${process.platform}-${process.arch}` | ||
|
||
// If the fix worked correctly we should have installed the prebuilt binary for our platform under our `.cache` directory | ||
const lmdbRequire = mod.createRequire( | ||
path.resolve(rootPath, ".cache", "internal-packages", "package.json") | ||
) | ||
expect(() => { | ||
lmdbRequire.resolve(lmdbPackage) | ||
}).not.toThrow() | ||
|
||
// The resulting query-engine bundle should not contain the binary built from source | ||
const binaryBuiltFromSource = path.resolve( | ||
rootPath, | ||
".cache", | ||
"query-engine", | ||
"assets", | ||
"build", | ||
"Release", | ||
"lmdb.node" | ||
) | ||
expect(fs.existsSync(binaryBuiltFromSource)).toEqual(false) | ||
}) | ||
}) |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,9 @@ | ||
module.exports = { | ||
siteMetadata: { | ||
siteUrl: `https://www.yourdomain.tld`, | ||
}, | ||
plugins: [], | ||
flags: { | ||
DEV_SSR: true, | ||
}, | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
module.exports = { | ||
testPathIgnorePatterns: [`/node_modules/`, `__tests__/fixtures`, `.cache`], | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,22 @@ | ||
{ | ||
"name": "lmdb-regeneration", | ||
"private": true, | ||
"author": "Sid Chatterjee", | ||
"description": "A simplified bare-bones starter for Gatsby with DSG", | ||
"version": "0.1.0", | ||
"license": "MIT", | ||
"scripts": { | ||
"build": "gatsby build", | ||
"clean": "gatsby clean", | ||
"test": "jest --runInBand" | ||
}, | ||
"dependencies": { | ||
"gatsby": "next", | ||
"react": "^18.2.0", | ||
"react-dom": "^18.2.0" | ||
}, | ||
"devDependencies": { | ||
"fs-extra": "^11.1.0", | ||
"jest": "^29.3.1" | ||
} | ||
} |
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,49 @@ | ||
import * as React from "react" | ||
import { Link } from "gatsby" | ||
|
||
const pageStyles = { | ||
color: "#232129", | ||
padding: "96px", | ||
fontFamily: "-apple-system, Roboto, sans-serif, serif", | ||
} | ||
const headingStyles = { | ||
marginTop: 0, | ||
marginBottom: 64, | ||
maxWidth: 320, | ||
} | ||
|
||
const paragraphStyles = { | ||
marginBottom: 48, | ||
} | ||
const codeStyles = { | ||
color: "#8A6534", | ||
padding: 4, | ||
backgroundColor: "#FFF4DB", | ||
fontSize: "1.25rem", | ||
borderRadius: 4, | ||
} | ||
|
||
const NotFoundPage = () => { | ||
return ( | ||
<main style={pageStyles}> | ||
<h1 style={headingStyles}>Page not found</h1> | ||
<p style={paragraphStyles}> | ||
Sorry 😔, we couldn’t find what you were looking for. | ||
<br /> | ||
{process.env.NODE_ENV === "development" ? ( | ||
<> | ||
<br /> | ||
Try creating a page in <code style={codeStyles}>src/pages/</code>. | ||
<br /> | ||
</> | ||
) : null} | ||
<br /> | ||
<Link to="/">Go home</Link>. | ||
</p> | ||
</main> | ||
) | ||
} | ||
|
||
export default NotFoundPage | ||
|
||
export const Head = () => <title>Not found</title> |
17 changes: 17 additions & 0 deletions
17
integration-tests/lmdb-regeneration/src/pages/test-dsg.jsx
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,17 @@ | ||
import * as React from "react"; | ||
|
||
const DSG = () => { | ||
return <h1>DSG</h1>; | ||
}; | ||
|
||
export default DSG; | ||
|
||
export const Head = () => <title>DSG</title>; | ||
|
||
export async function config() { | ||
return () => { | ||
return { | ||
defer: true, | ||
}; | ||
}; | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
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
withomit=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?There was a problem hiding this comment.
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 abuild
from source in thelmdb
directory? (I can dig through the code to try to understand that but tbh I worry that we might miss something by doing so)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 isinstall
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 ofnode-gyp-build(-optional-packages)
fromlmdb
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:gatsby/packages/gatsby/src/schema/graphql-engine/lmdb-bundling-patch.ts
Lines 74 to 85 in 22c2412
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 uselmdb
instance from there currently - we could add webpack alias to use that instance oflmdb
package for lambda, but my biggest hang up with it is that installing mainlmdb
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:in
.npmrc
would also likely cause the issue (similarly toomit = optional
)