Move tar outside of dependencies #338
Conversation
|
|
TwistedTwigleg
added
the
pr/do-not-merge
… move-tar-cmake-outside
…installed and only if we need them to make a build. This makes them a pseudo dependency, but it should not download for those that either have cmake-js already in their node_modules or do not need to install from source
This comment was marked as outdated.
This comment was marked as outdated.
TwistedTwigleg
removed
the
pr/do-not-merge
TwistedTwigleg
changed the title
… move-tar-cmake-outside
scripts/build.js
Outdated
| console.log("Downloading " + package_name + " from npm for build..."); | ||
| // Try to intall the given package and ONLY the given package. Will throw an exception if there is an error. | ||
| if (package_version != null) { | ||
| child_process.execSync("npm install --no-package-lock --save-dev --ignore-scripts " + package_name + "@" + package_version); |
There was a problem hiding this comment.
This seems really sketchy
I realize the whole premise of downloading code and compiling it as part of the install process is already a tough pill to swallow, but is it going to far to install additional NPM dependencies that aren't declared in package.json?
Let's talk with someone that has deeper familiarity with what is considered acceptable in Javascript-Land
But if this is acceptable, we could do it with cmake-js as well 🤷♀️
There was a problem hiding this comment.
I will leave it for now until and see if I can get more feedback to make sure the change is okay with those experienced in Javascript 👍
There was a problem hiding this comment.
For good or bad, this doesn't faze me as a JS developer.
I suggest doing this in a subfolder workspace such as your_package_location/scripts/.
You can leave a dummy package.json with private:true which will allow temporary modules to be installed there if they are not part of the user's workspace.
That way, the workflow could look like this:
- user runs
npm install aws-crt - the
aws-crtpackage is downloaded to their node_modules (root)aws-crtpostinstall tries to use the rootcmake-jset. al. runtime packages- if not present or of the wrong version, download them into a subfolder of
aws-crtso as not to affect the user's root workspace node_modules / package.json / package-lock.json etc. - if not able to download, let the user know they will need to install certain dev dependencies and re-run the
aws-crtinstall script.
This would avoid bringing non-runtime packages into both the user's development workspace (root node_modules) as well as the user's potential runtime distribution (package.json->dependencies).
scripts/build.js
Outdated
|
|
||
| // Do we have it in node modules? If so, then use that! | ||
| try { | ||
| if (fs.existsSync("./node_modules/" + package_name)) { |
There was a problem hiding this comment.
you can use require.resolve if you want to check for a module and aren't specifically looking in the immediate node_modules directory.
This comment was marked as outdated.
This comment was marked as outdated.
…nd use npm-list, added better logging
This comment was marked as outdated.
This comment was marked as outdated.
|
|
||
| axios: null, | ||
| clean_up_axios: false, | ||
| axios_version: "0.24.0", |
There was a problem hiding this comment.
I am not a fan of this hard-coded version...
- We cannot tell if any CVE or something happens to that package at that version unless manually checking the code, which could be a concern
- If the lib was existed, and it's not this version, we will not know.
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.
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 the scripts/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.
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.
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 the package.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.
|
|
||
| axios: null, | ||
| clean_up_axios: false, | ||
| axios_version: "0.24.0", |
There was a problem hiding this comment.
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.🫣
…from there, actually fix tar and that whole download path for good
|
Thanks for the reviews everyone! Merging into |
Description of changes:
Mirror of #335 but merged with latest changes inmainand the package lock regenerated with npm.Moves
cmake-jsandtaroutside of dependencies and modifies the build script to downloadtarandcmake-js, use it, and store it in a sub-folder for reuse that is completely separate from the main node modules. This preventscmake-jsandtarfrom sticking around when it is only needed for building the source and not in the functionality of the CRT itself (and in the case oftar, it is needed only for a single download once to get the source).With this change,
cmake-jsandtaris not sticking around in the node dependencies forever when it is only needed for build-time operations.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.