node-gradle / gradle-node-plugin

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

Environment variables that are non-input properties (non-change triggering incremental builds) #187

Closed dweiss closed 3 years ago

dweiss commented 3 years ago

Hello. We have a scenario where npm builds take environment variables such as the build timestamp. These really are non-essential to the task's input/output and currently they cause costly rebuilds on each and every run because the timestamp value changes and is propagated as an input property.

I've been wondering if there is an elegant solution to this other than:

1) using the internal execOverrides to pass those properties, 2) messing with gradle's onlyIf (problematic; you'd need to maintain the entire logic of checking inputs/ outputs).

Previously we used a hack where these properties were set once the task was executed (so they were not part of the input) but this is currently not possible with the lazy properties API.

Any ideas would be welcome.

dweiss commented 3 years ago

Oh, I just wanted to add that a trival plugin-side solution would be to add a second property like "nonInputEnvironment" that wouldn't be marked as an input and merge the two during execution... Or, alternatively, a customizable filter that would expose only a subset of the environment's contents as an input. Both of these require plugin changes though so if there is an external better solution, I'd love to hear about one.

deepy commented 3 years ago

I think the easiest way is to add a Closure/Action that gets to decide which ones to expose to Gradle for up-to-date checking, I might be able to look into this later today

dweiss commented 3 years ago

Thanks @deepy ! Indeed, it'd solve the problem.

deepy commented 3 years ago

If anyone else is interested in looking at this it should be fairly simple to do any of the two following parts: A. Add (_integTest) tests that show this is currently not working (e.g. one for each of NpmTask, NodeTask, YarnTask, etc) B. For each of the above mentioned, annotate val environment as @Internal, add an Action<String> or Action<Map.Entry<String, String>> for making decisions on which ones to expose to Gradle.

For correctness sake we probably shouldn't pass the ones that are filtered out to npm/node/etc but I also suspect there's people who want them to get passed on to the thing, at the moment I don't have any strong feelings on either

deepy commented 3 years ago

@dweiss just to confirm something, are you setting the environment variable on the task or are you setting it outside of Gradle before starting the build?

If it's the latter I misunderstood and then execOverrides is the preferred way, something like the following should work for adding things that Gradle shouldn't look at:

execOverrides = Action { getEnvironment().put("timestamp", "today") }
execOverrides = { getEnvironment().put("timestamp", "today") }
dweiss commented 3 years ago

It's an environment property on the task, @deepy . So yes, execOverrides works for me (and it's the current workaround I implemented). I just wasn't sure whether this property isn't something internal for tests only. If it's not, then this is a good workaround and the issue can be closed.

deepy commented 3 years ago

Yeah execOverrides is public, the plugin has opinionated ways of doing things and execOverrides is our way of allowing users to say "I know better" or "I need to do something in a different way" so it's fine to use :-)

dweiss commented 3 years ago

Thanks for confirming, @deepy !