search

paketo-buildpacks / rfcs

Apache License 2.0
19 stars 33 forks source link

Implement RFC0019: Default Behaviour for Buildpack-Set Language Ecosystem Environment Variables #64

Open ryanmoran opened 3 years ago

ryanmoran commented 3 years ago

RFC

Summary

Paketo buildpacks sometimes use language ecosystem environment variables to configure build- and launch time behaviour. The environment variables' values can come a) from user input at build/launch time or b) from buildpacks' "opinions" about proper settings for the container build. If a user provides a language ecosystem environment variable at build time and the buildpack typically sets an opinionated build time value, the user's value should override or have precedence over the buildpack-set value. Likewise if a user provides a language ecosystem environment variable at launch time.

In addition, if a user provides a language-ecosystem environment variable at build time, that value should not be "sticky" -- it should not influence the launch time value of the environment variable. If a user wants to change the launch time value of an environment variable, they should provide it at launch time.

fg-j commented 2 years ago

Given that this RFC sets a norm rather than requiring a change, will we ever close this issue?

ryanmoran commented 2 years ago

I believe the intent here is to ensure we take the time to survey the buildpacks we currently have and ensure they comply with what is outlined in the RFC. From that point, we can consider this RFC implemented with the expectation that buildpacks will remain in compliance.

fg-j commented 2 years ago

A list of buildpacks that still use the Override environment variable option for language ecosystem variables:

dmikusa commented 2 years ago

A list of buildpacks that still use the Override environment variable option for language ecosystem variables:

  • [x] libjvm

    • sets JAVA_HOME and JDK_HOME; @paketo-buildpacks/java-buildpacks is there a compelling reason why buildpack users shouldn't be able to override this value at build time?

  • [x] graalvm

    • likewise, sets JAVA_HOME and JDK_HOME

This is the location where the buildpack installs Java/ the JDK. A user would not need to change this (and probably wouldn't know the correct path under /layers/...) unless they were providing their own Java install.

There are two cases where I could see this as a possibility:

  1. If a user creates their own build and run images that include Java/JDK. I don't think you could set the env variables, but it may not be necessary as you could ensure that java is on the $PATH through other means. I haven't done this though, so it may still be necessary to have something set those env variables.
  2. Buildpack extensions when those are implemented & released. The extension would need to set these env variables.

Regardless, I think that the expectation is that only one thing is providing Java. Right now, you can have 10 different buildpacks all providing Java, but only one of them ever runs and satisfies that part of the buildplan.

fg-j commented 2 years ago

@dmikusa-pivotal Based on the letter of the RFC as it's written now,

If a user provides a language ecosystem environment variable at build time and the buildpack typically sets an opinionated build time value, the user's value should override or have precedence over the buildpack-set value.

In this case, JAVA_HOME and JDK_HOME are "language ecosystem environment variables". Given what you've explained about the role of the env vars, it sounds like using the "Default" env var option instead of the "Override" option wouldn't break builds, but might open the door for advanced users to do weird stuff. So I think swapping to use the "Default" env var option makes sense in this case to comply with the RFC in letter and spirit. If you feel differently, let's hash out an RFC amendment to capture the nuance of this type of case!

dmikusa commented 2 years ago

Given what you've explained about the role of the env vars, it sounds like using the "Default" env var option instead of the "Override" option wouldn't break builds, but might open the door for advanced users to do weird stuff. So I think swapping to use the "Default" env var option makes sense in this case to comply with the RFC in letter and spirit. If you feel differently, let's hash out an RFC amendment to capture the nuance of this type of case!

I don't think it should break anything to switch to DEFAULT, although I've not validated or tested that, but it will enable users to break things more easily. I think the reason it's this way now is an attempt to protect users. I'm not necessarily opposed to changing to DEFAULT and allowing users to configure this/potentially shoot themselves in the foot. I just don't think we'd prioritize the work unless there was a user request for it. If you want to add an issue under libjvm, we can use that to track the request.

GraalVM should be OK. There are some other refactorings happening and I think that code will go away as it'll start using libjvm.

ForestEckhardt commented 2 years ago

As outlined in this RFC https://github.com/paketo-buildpacks/rfcs/pull/219 the goal is to move away from having to set the DOTNET_ROOT environment variable entirely.

ForestEckhardt commented 2 years ago

  • yarn-install

    • sets NODE_ENV=development on the layer that contains build-time node modules. @paketo-buildpacks/nodejs-maintainers, would this feature still work if we used BuildEnv.Default instead?

  • [ ] npm-install

    • sets NODE_ENV=development on the layer that contains build-time node modules. @paketo-buildpacks/nodejs-maintainers, would this feature still work if we used BuildEnv.Default instead?

Upon some investigation on this issue I discovered that part of the problem lies in the node-engine buildpack which sets NODE_ENV=production as a default. This is an issue because the spec says the following:

Earlier buildpacks' environment default variable file contents MUST override later buildpacks' environment variable file contents.

This means that yarn-install and npm-install have to set NODE_ENV=development as an override for build for the environment variable to actually function. We want to set NODE_ENV=development because it is my understanding some npm and yarn commands will not use devDependencies at all if NODE_ENV is not set to development. I would point out that the only scenario in which node-modules are required at build time is when some form of node script is being run which is the case for things like Javascript Frontend compiled apps.

I am not sure what the best course of action is but that is the context that I have been able to find.

dmikusa commented 2 years ago

Graalvm is using libjvm now, so it's no longer unique. For libjvm, we had a short discussion and no one could come up with a reason that a user would want to override one of these env variables. Since it could inadvertently allow a user to more easily break stuff, we're going to leave them as they are for now, using OVERRIDE. If someone raises a legitimate use case for DEFAULT, we'll make the change.

fg-j commented 2 years ago

Ping @paketo-buildpacks/ruby-maintainers and @paketo-buildpacks/nodejs-maintainers where are we on this?

robdimsdale commented 1 year ago

[ ] mri

  • sets GEM_PATH . @paketo-buildpacks/ruby-maintainers, would it be reasonable to set that env var as a default instead?

We haven't got to this yet, but I'm happy to do it. On the one hand, I can't see a situation in which changing this is useful. On the other hand I don't mind giving users the opportunity to change it if they have a reason which I don't currently know about. @paketo-buildpacks/ruby-maintainers any objections to changing GEM_PATH from Override to Default?

ForestEckhardt commented 1 year ago

@paketo-buildpacks/nodejs-maintainers & @paketo-buildpacks/ruby-maintainers Could y'all take a look and reexamine your positions on this?

robdimsdale commented 1 year ago

Yeah, it's been a while 😬

On the ruby side we'll get to it shortly.