-
Notifications
You must be signed in to change notification settings - Fork 27
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
Move tar outside of dependencies #338
Merged
Merged
Changes from 13 commits
Commits
Show all changes
17 commits
Select commit
Hold shift + click to select a range
517545c
Move cmake-js and tar to devDeps
trivikr 4d173d7
Merge branch 'move-tar-cmake-outside' of https://github.com/trivikr/a…
TwistedTwigleg e291900
Regenerated the package lock after merging changes to main
TwistedTwigleg 3fc6f15
Merge branch 'main' of https://github.com/awslabs/aws-crt-nodejs into…
TwistedTwigleg 893f8cf
Adjust build script to download cmakejs and tar only if they are not …
TwistedTwigleg b8bdeb5
Fix LGTM warning
TwistedTwigleg 8ddd8e3
Add cmake-js back as a dependency
TwistedTwigleg b94c2f8
Merge branch 'main' of https://github.com/awslabs/aws-crt-nodejs into…
TwistedTwigleg 8ac1fee
Add a try-catch around downloading tar
TwistedTwigleg 84a9b6d
Initial version of having a build-specific set of NPM dependencies
TwistedTwigleg b8a9e96
Documented axios build step file, adjusted code to look for version a…
TwistedTwigleg 0d2a3b0
Merge branch 'main' into move-tar-cmake-outside
TwistedTwigleg 47e1063
Fix LGTM warnings by removing unused imports
TwistedTwigleg e583e28
Code review changes and general code cleanup
TwistedTwigleg c5b8868
Fix copy-paste error in tar build step
TwistedTwigleg f65dfc4
Specify the version of tar to use
TwistedTwigleg e9382cf
Move the runtime dependencies to devDependencies and get the version …
TwistedTwigleg 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
Large diffs are not rendered by default.
Oops, something went wrong.
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 |
---|---|---|
|
@@ -3,7 +3,10 @@ | |
"version": "1.0.0-dev", | ||
"description": "NodeJS/browser bindings to the aws-c-* libraries", | ||
"homepage": "https://github.com/awslabs/aws-crt-nodejs", | ||
"repository": "github:awslabs/aws-crt-nodejs", | ||
"repository": { | ||
"type": "git", | ||
"url": "git+https://github.com/awslabs/aws-crt-nodejs.git" | ||
}, | ||
"contributors": [ | ||
"AWS Common Runtime Team <[email protected]>" | ||
], | ||
|
@@ -46,9 +49,7 @@ | |
"@aws-sdk/util-utf8-browser": "^3.109.0", | ||
"@httptoolkit/websocket-stream": "^6.0.0", | ||
"axios": "^0.24.0", | ||
"cmake-js": "6.3.0", | ||
"crypto-js": "^4.0.0", | ||
"mqtt": "^4.3.7", | ||
"tar": "^6.1.11" | ||
"mqtt": "^4.3.7" | ||
} | ||
} |
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,106 @@ | ||
/** | ||
* Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. | ||
* SPDX-License-Identifier: Apache-2.0. | ||
*/ | ||
const fs = require("fs"); | ||
const crypto = require('crypto'); | ||
const utils = require('./build_utils'); | ||
|
||
module.exports = { | ||
|
||
axios: null, | ||
clean_up_axios: false, | ||
axios_version: "0.24.0", | ||
|
||
/** | ||
* Loads the axios library. We want to do this seperate instead of having a performStep function | ||
* because the axios library is needed for multiple functions that have different data passed to them. | ||
*/ | ||
loadAxios: function () { | ||
const workDir = path.join(__dirname, "../../") | ||
|
||
process.chdir(__dirname); | ||
if (this.axios == null) { | ||
if (utils.npmCheckIfPackageExists("axios", this.axios_version) == true) { | ||
this.axios = require("axios"); | ||
} else { | ||
try { | ||
this.clean_up_axios = utils.npmDownloadAndInstallRuntimePackage("axios", this.axios_version); | ||
this.axios = require('axios'); | ||
} catch (error) { | ||
console.log("ERROR: Could not download axios! Cannot build CRT"); | ||
console.log("Please install axios verion " + this.axios_version + " and then run the aws-crt install script again"); | ||
process.exit(1); | ||
} | ||
} | ||
} | ||
process.chdir(workDir); | ||
}, | ||
|
||
/** | ||
* Downloads the file from the given file URL and places it in the given output location path. | ||
* @param {*} fileUrl The file to download | ||
* @param {*} outputLocationPath The location to store the downloaded file | ||
* @returns A promise for the file download | ||
*/ | ||
downloadFile: function (fileUrl, outputLocationPath) { | ||
const writer = fs.createWriteStream(outputLocationPath); | ||
return this.axios({ | ||
method: 'get', | ||
url: fileUrl, | ||
responseType: 'stream', | ||
}).then(response => { | ||
return new Promise((resolve, reject) => { | ||
response.data.pipe(writer); | ||
let error = null; | ||
writer.on('error', err => { | ||
error = err; | ||
writer.close(); | ||
reject(err); | ||
}); | ||
writer.on('close', () => { | ||
if (!error) { | ||
resolve(); | ||
} | ||
}); | ||
}); | ||
}); | ||
}, | ||
|
||
/** | ||
* Performs a checksum check on the given file. The checksum is downloaded from the given URL | ||
* and then the file given is checked using said checksum. | ||
* @param {*} url The URL containing the checksum | ||
* @param {*} local_file The file to check | ||
* @returns A promise for the result of the check | ||
*/ | ||
checkChecksum: function (url, local_file) { | ||
return this.axios({ | ||
method: 'get', | ||
url: url, | ||
responseType: 'text', | ||
}).then(response => { | ||
return new Promise((resolve, reject) => { | ||
const filestream = fs.createReadStream(local_file); | ||
const hash = crypto.createHash('sha256'); | ||
filestream.on('readable', () => { | ||
// Only one element is going to be produced by the | ||
// hash stream. | ||
const data = filestream.read(); | ||
if (data) | ||
hash.update(data); | ||
else { | ||
const checksum = hash.digest("hex") | ||
if (checksum === response.data) { | ||
resolve() | ||
} | ||
else { | ||
reject(new Error("source code checksum mismatch")) | ||
} | ||
} | ||
}); | ||
}); | ||
}) | ||
} | ||
|
||
} |
Oops, something went wrong.
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 am not a fan of this hard-coded version...
But, I cannot think of a better way😞.
Does any other nodejs lib using the similar approach to fetch dependency dynamically? How do they deal with the version?
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'm not sure, I would need to look around and see.
We could put it in our dev dependencies and then pull it out of the
package.json
that way. Then it would get automatically code scanned by Dependabot.Though the dependencies here are completely isolated from the rest of the node modules by being in the
node_modules
folder of thescripts/build_dependencies
folder. So even if there was a vulnerability, it would only be for building the source and not the output or anything a customer/consumer of the CRT would see.Not saying it is something we shouldn't fix, but the security impact is minimal because it just gets downloaded and used only for a build.
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 am not totally against it. I think we need to do more research, or how do JS experts think about this? If they think it's okay, just ignore me by all means.🫣
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 did some looking, and it looks like the general consensus is "don't do it" based on other Javascript packages and such. I got around this by putting it in the
devDependencies
and then loading thepackage.json
and getting the version number from there. Now we have the advantage of getting Dependabot code scanning, can easily change the version in a single place, and it is more in line with what I have seen in other Javascript packages/modules.