Skip to content
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

Warn if the environment of the client does not match the environment of the daemon #122

Closed
zbendhiba opened this issue Oct 21, 2020 · 18 comments
Milestone

Comments

@zbendhiba
Copy link

Hello, I have 3 different versions of graalvm install on my linux machine (RHEL 8).
while usen mvn, it takes the version in $GRAALVM_HOME.
But when I use mvnd, it uses a different version. Is it an issue, or do I need to configure something? How can I switch to another version of GraalVm ?

FYI, I was compiling the quarkus way!!

@gnodet
Copy link
Contributor

gnodet commented Oct 21, 2020

The java version used to build is determined by the JAVA_HOME environment variable.
You can see which one is used by running mvnd -v which should output something like:

Maven Daemon 0.0.8-darwin-amd64 (native)
Apache Maven 3.6.3 (cecedd343002696d0abb50b32b541b8a6ba2883f)
Maven home: /Users/gnodet/.sdkman/candidates/mvnd/current/mvn
Java version: 11.0.8, vendor: AdoptOpenJDK, runtime: /Users/gnodet/.sdkman/candidates/java/11.0.8.hs-adpt
Default locale: en_GB, platform encoding: UTF-8
OS name: "mac os x", version: "10.15.7", arch: "x86_64", family: "Mac"

@zbendhiba
Copy link
Author

Ok setting GRAALVM_HOME on JAVA_HOME works fine !!
Thanks

@ppalaga
Copy link
Contributor

ppalaga commented Oct 21, 2020

I think this is a general problem with environment variables that we should document at least. The daemon is inheriting the env from the first client that starts it. The env of any subsequent mvnd client executions is ignored and that's something we cannot fix in any way. (Is that right, @gnodet?)

JAVA_HOME is a bit exceptional, because the client takes care to choose a matching daemon and if none is available, it starts a new one.

Thus the following does not work:

export GRAALVM_HOME=/some/path
mvnd ... # initial invocation starts a daemon with GRAALVM_HOME=/some/path
export GRAALVM_HOME=/other/path
mvnd .... # the running daemon still sees GRAALVM_HOME=/some/path

But the following should work

export GRAALVM_HOME=/some/path
mvnd ... # initial invocation starts a daemon with GRAALVM_HOME=/some/path
export GRAALVM_HOME=/other/path
mvnd --stop # kill all running daemons
mvnd ... # starts a fresh daemon with GRAALVM_HOME=/other/path

@famod
Copy link
Contributor

famod commented Oct 21, 2020

I wonder whether the daemon (or client?) should compare the current environment to the initial one and issue a warning in case there are differences? Or even fail?

@ppalaga
Copy link
Contributor

ppalaga commented Oct 21, 2020

I'd say a warning is enough, because not every change in the env matters.
A warning with a recommendation to kill the daemon is easy to do.

@gnodet
Copy link
Contributor

gnodet commented Oct 21, 2020

We could also pass the environment to the daemon and try to set it after displaying a warning.

Alternatively, we could create a strategy so that the user can choose between various strategies : ignore (do not care about any differences) / warnAndSet (display a warning if there are some changes and try to override the JVM environment) / reject (creates a new daemon).

The following code snippet should change the JVM environment. It should be conveyed to child processes started using the ProcessBuilder.

protected static void setEnv(Map<String, String> newenv) throws Exception {
  try {
    Class<?> processEnvironmentClass = Class.forName("java.lang.ProcessEnvironment");
    Field theEnvironmentField = processEnvironmentClass.getDeclaredField("theEnvironment");
    theEnvironmentField.setAccessible(true);
    Map<String, String> env = (Map<String, String>) theEnvironmentField.get(null);
    env.putAll(newenv);
    Field theCaseInsensitiveEnvironmentField = processEnvironmentClass.getDeclaredField("theCaseInsensitiveEnvironment");
    theCaseInsensitiveEnvironmentField.setAccessible(true);
    Map<String, String> cienv = (Map<String, String>)     theCaseInsensitiveEnvironmentField.get(null);
    cienv.putAll(newenv);
  } catch (NoSuchFieldException e) {
    Class[] classes = Collections.class.getDeclaredClasses();
    Map<String, String> env = System.getenv();
    for(Class cl : classes) {
      if("java.util.Collections$UnmodifiableMap".equals(cl.getName())) {
        Field field = cl.getDeclaredField("m");
        field.setAccessible(true);
        Object obj = field.get(env);
        Map<String, String> map = (Map<String, String>) obj;
        map.clear();
        map.putAll(newenv);
      }
    }
  }
}

@ppalaga
Copy link
Contributor

ppalaga commented Oct 21, 2020

I do not think the issue is critical enough for trying to change the env of a running daemon using reflection. I vote for present this as a limitation given by how the daemon works.

@gnodet
Copy link
Contributor

gnodet commented Oct 21, 2020

I do not think the issue is critical enough for trying to change the env of a running daemon using reflection. I vote for present this as a limitation given by how the daemon works.

I don't have any objection. However the hard part is the detection of the mismatch : we'd need to either store the daemon's environment in the library, or preferably, have the client send it to the daemon so that the daemon can detect mismatches and display warnings.

@ppalaga
Copy link
Contributor

ppalaga commented Oct 21, 2020

the client send it to the daemon so that the daemon can detect mismatches and display warnings.

Yes, I am in favor of this.

gnodet added a commit to gnodet/mvnd that referenced this issue Oct 22, 2020
ppalaga added a commit that referenced this issue Oct 22, 2020
Display warning in case of environment mismatch #122
@ppalaga ppalaga changed the title mvnd graalvm version Environment variables not propagated to the daemon during subsequent builds Oct 23, 2020
@ppalaga
Copy link
Contributor

ppalaga commented Oct 23, 2020

#125 adds a warning - that's all we can do.

@ppalaga ppalaga closed this as completed Oct 23, 2020
@famod
Copy link
Contributor

famod commented Oct 23, 2020

@ppalaga Haven't tried it myself but is there one daemon per...hm...project path?
Meaning: What happens if you execute mvnd in /foo and then in /bar (also possibly in another shell instance with different env vars)?

@ppalaga
Copy link
Contributor

ppalaga commented Oct 23, 2020

@ppalaga Haven't tried it myself but is there one daemon per...hm...project path?

No, only the Java version must match.

Meaning: What happens if you execute mvnd in /foo and then in /bar (also possibly in another shell instance with different env vars)?

The behavior is described above. We have recently added a warning if the environments of the client and daemon are different.

@famod
Copy link
Contributor

famod commented Oct 23, 2020

So that means that you cannot use mvnd (without killing the daemon) to build multiple projects concurrently or shortly after one another, e.g. smallrye-config and then quarkus, in case the environment is different?
One example would be different MAVEN_OPTS.

What happens if you have multiple mvnd installations?
At work we have a Maven installation per git clone (e.g. to work on different releases without switching branches all the time) and each installation has a "private" local repo that is set via conf/settings.xml.
Would that work with mvnd?

@gnodet
Copy link
Contributor

gnodet commented Oct 23, 2020

So that means that you cannot use mvnd (without killing the daemon) to build multiple projects concurrently or shortly after one another, e.g. smallrye-config and then quarkus, in case the environment is different?
One example would be different MAVEN_OPTS.

What kind of MAVEN_OPTS ? Like configuring memory ? Currently, those are not passed to the daemon anyway. The selection of a daemon is currently based only on the JAVA_HOME environment variable. You can very well build two different projects with the same daemon, the environment variables usually don't matter much, but if you have any specific thoughts, let me know.

What happens if you have multiple mvnd installations?
At work we have a Maven installation per git clone (e.g. to work on different releases without switching branches all the time) and each installation has a "private" local repo that is set via conf/settings.xml.
Would that work with mvnd?

There should be no problem in using different installations of mvnd. Each one will manage a different set of daemon processes.

@ppalaga
Copy link
Contributor

ppalaga commented Oct 24, 2020

I think we could handle MAVEN_OPTS as a special case. @famod could you please file a separate issue for that?

@gnodet
Copy link
Contributor

gnodet commented Oct 24, 2020

I think we could handle MAVEN_OPTS as a special case. @famod could you please file a separate issue for that?

JVM memory settings should be made confiurable, but I don't think we should pass them to the daemon. System properties are sent. If environment variables seem to be an issue, we now already send them, so it would be easy to override the environment instead. I'd like to better understand the real use case before implementing anything here.

@ppalaga
Copy link
Contributor

ppalaga commented Oct 24, 2020

With stock Maven, the content of MAVEN_OPTS is passed as an arg to the java executable. That happens in the mvn shell script.

Our mvnd.sh also passes MAVEN_OPTS to the client JVM, but the client JVM does not use it in any way when starting the daemon.

Our mvnd native executable does not care for MAVEN_OPTS at all.

I guess users may expect mvnd clients to honor MAVEN_OPTS when starting a new daemon at least.

@ppalaga
Copy link
Contributor

ppalaga commented Oct 24, 2020

I guess users may expect mvnd clients to honor MAVEN_OPTS when starting a new daemon at least.

I do not say that's necessarily a good idea, but we should at least consider having a mechanism for starting the daemon with custom JVM args.

@ppalaga ppalaga changed the title Environment variables not propagated to the daemon during subsequent builds Warn if environment of the client does not match the environment of the daemon Oct 25, 2020
@ppalaga ppalaga added this to the 0.0.9 milestone Oct 25, 2020
@ppalaga ppalaga changed the title Warn if environment of the client does not match the environment of the daemon Warn if the environment of the client does not match the environment of the daemon Oct 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants