node-gradle / gradle-node-plugin

Gradle plugin for integrating NodeJS in your build. :rocket:
Apache License 2.0
601 stars 117 forks source link

Add option download auto #221

Open sytolk opened 2 years ago

sytolk commented 2 years ago

We can have option auto here: https://github.com/node-gradle/gradle-node-plugin/blob/master/docs/usage.md#configuring-the-plugin

look at the comments

node {
    // options: yes | no | auto
    // Whether to download and install a specific Node.js version or not
    // If "no", it will use the globally installed Node.js
    // If "yes", it will download node using above parameters
    // Note that npm is bundled with Node.js
    // If "auto", it will check and download node only if its mising (auto will check if node is installed and switch between yes and no modes)

    download = 'auto'
}

In CI environment there is no sense to download node if its already exist. If node exist and its downloaded another version of node from this plugin its have versions conflicts in alpine: https://github.com/node-gradle/gradle-node-plugin/issues/51

for this issue with download = true :

#8 141.5 FAILURE: Build failed with an exception.
#8 141.5 
#8 141.5 > Task :webpack FAILED
#8 141.5 * What went wrong:
#8 141.5 Execution failed for task ':webpack'.
#8 141.5 > A problem occurred starting process 'command '/.gradle/nodejs/node-v16.14.0-linux-x64/bin/node''

my temp fix is to set download = false and install node manually apk add --update nodejs npm

deepy commented 2 years ago

But if your CI environment already has node and npm installed shouldn't you be using download = false?

If your CI environment has some environment variable set (like CI) you could use that to set download to the appropriate value

sytolk commented 2 years ago

Yes of course I can switch download = false/true based on ENV=CI but this takes time.. download=auto will skip this checks in build.gradle for different environments (this can be the default option)

An other option is to detect musl for linux alpine and alwayse setdownload = false with message for devs like -> "We cannot download node musl for this system please install it manually"

deepy commented 2 years ago

I'm not sure why an environment variable check would take time, but I think I can confidently say that implementing that will be faster than building the detection functionality in the plugin. And currently download = false just assumes node is on the path and download = true will just add node to the path, so to add it there would have to be some significant refactoring.

And while I have thought about the musl detection and erroring out, that'd prevent you from using your own internal mirror which could contain musl-compatible builds (and I think there's an unofficial repository which is publicly available and does this already)

Although all of this will be irrelevant if I get the Shared Build Services approach to work (there's some issues related to it)

And while download = auto is a good idea in general the specifics of the code in the plugin means that it's more work to support than the build service approach (which would have the same effect in the end)