Skip to content

Commit

Permalink
A new feature 'excludedEnvironmentVariables' (pending: MOJO system pr…
Browse files Browse the repository at this point in the history
…operties with prefix 'surefire' and 'failsafe').
  • Loading branch information
Tibor17 committed Aug 3, 2019
1 parent c6694ed commit 065001b
Show file tree
Hide file tree
Showing 15 changed files with 357 additions and 45 deletions.
Expand Up @@ -444,6 +444,21 @@ public abstract class AbstractSurefireMojo
@Parameter
private Map<String, String> environmentVariables = new HashMap<>();

/**
* You can selectively exclude individual environment variables by enumerating their keys.
* <br>
* All environment variables of the plugin process are inherited to a Surefire sub-process by default.
* The keys must literally (case sensitive) match in order to exclude their environment variable.
* <br>
* The environment is a system-dependent mapping from keys to values which is passed from parent plugin process
* to the forked Surefire processes.<br>
* Example to exclude two environment variables: <i>mvn test -DexcludedEnvironmentVariables=ACME1,ACME2</i>
*
* @since 3.0.0-M4
*/
@Parameter( property = "excludedEnvironmentVariables" )
private String[] excludedEnvironmentVariables;

/**
* Command line working directory.
*
Expand Down Expand Up @@ -1195,6 +1210,7 @@ private RunResult executeProvider( @Nonnull ProviderInfo provider, @Nonnull Defa
if ( getConsoleLogger().isDebugEnabled() )
{
showMap( getEnvironmentVariables(), "environment variable" );
showArray( getExcludedEnvironmentVariables(), "excluded environment variable" );
}

Properties originalSystemProperties = (Properties) System.getProperties().clone();
Expand Down Expand Up @@ -2268,6 +2284,7 @@ private ForkConfiguration createForkConfiguration( Platform platform )
getProject().getModel().getProperties(),
getArgLine(),
getEnvironmentVariables(),
getExcludedEnvironmentVariables(),
getConsoleLogger().isDebugEnabled(),
getEffectiveForkCount(),
reuseForks,
Expand All @@ -2283,6 +2300,7 @@ else if ( getClassLoaderConfiguration().isManifestOnlyJarRequestedAndUsable() )
getProject().getModel().getProperties(),
getArgLine(),
getEnvironmentVariables(),
getExcludedEnvironmentVariables(),
getConsoleLogger().isDebugEnabled(),
getEffectiveForkCount(),
reuseForks,
Expand All @@ -2298,6 +2316,7 @@ else if ( getClassLoaderConfiguration().isManifestOnlyJarRequestedAndUsable() )
getProject().getModel().getProperties(),
getArgLine(),
getEnvironmentVariables(),
getExcludedEnvironmentVariables(),
getConsoleLogger().isDebugEnabled(),
getEffectiveForkCount(),
reuseForks,
Expand Down Expand Up @@ -2505,6 +2524,7 @@ private String getConfigChecksum()
checksum.add( getParallelTestsTimeoutInSeconds() );
checksum.add( getParallelTestsTimeoutForcedInSeconds() );
checksum.add( getEnvironmentVariables() );
checksum.add( getExcludedEnvironmentVariables() );
checksum.add( getWorkingDirectory() );
checksum.add( isChildDelegation() );
checksum.add( getGroups() );
Expand Down Expand Up @@ -2627,6 +2647,14 @@ private void showMap( Map<?, ?> map, String setting )
}
}

private <T> void showArray( T[] array, String setting )
{
for ( T e : array )
{
getConsoleLogger().debug( "Setting " + setting + " [" + e + "]" );
}
}

private Classpath getArtifactClasspath( Artifact surefireArtifact )
{
Classpath existing = ClasspathCache.getCachedClassPath( surefireArtifact.getArtifactId() );
Expand Down Expand Up @@ -3446,6 +3474,11 @@ public Map<String, String> getEnvironmentVariables()
return environmentVariables;
}

protected String[] getExcludedEnvironmentVariables()
{
return excludedEnvironmentVariables == null ? new String[0] : excludedEnvironmentVariables;
}

@SuppressWarnings( "UnusedDeclaration" )
public void setEnvironmentVariables( Map<String, String> environmentVariables )
{
Expand Down
Expand Up @@ -44,14 +44,15 @@ abstract class AbstractClasspathForkConfiguration
@Nonnull Properties modelProperties,
@Nullable String argLine,
@Nonnull Map<String, String> environmentVariables,
@Nonnull String[] excludedEnvironmentVariables,
boolean debug,
int forkCount,
boolean reuseForks,
@Nonnull Platform pluginPlatform,
@Nonnull ConsoleLogger log )
{
super( bootClasspath, tempDirectory, debugLine, workingDirectory, modelProperties, argLine,
environmentVariables, debug, forkCount, reuseForks, pluginPlatform, log );
environmentVariables, excludedEnvironmentVariables, debug, forkCount, reuseForks, pluginPlatform, log );
}

@Override
Expand Down
Expand Up @@ -44,12 +44,14 @@ public final class ClasspathForkConfiguration
public ClasspathForkConfiguration( @Nonnull Classpath bootClasspath, @Nonnull File tempDirectory,
@Nullable String debugLine, @Nonnull File workingDirectory,
@Nonnull Properties modelProperties, @Nullable String argLine,
@Nonnull Map<String, String> environmentVariables, boolean debug, int forkCount,
@Nonnull Map<String, String> environmentVariables,
@Nonnull String[] excludedEnvironmentVariables,
boolean debug, int forkCount,
boolean reuseForks, @Nonnull Platform pluginPlatform,
@Nonnull ConsoleLogger log )
{
super( bootClasspath, tempDirectory, debugLine, workingDirectory, modelProperties, argLine,
environmentVariables, debug, forkCount, reuseForks, pluginPlatform, log );
environmentVariables, excludedEnvironmentVariables, debug, forkCount, reuseForks, pluginPlatform, log );
}

@Override
Expand Down
Expand Up @@ -59,6 +59,7 @@ public abstract class DefaultForkConfiguration
@Nonnull private final Properties modelProperties;
@Nullable private final String argLine;
@Nonnull private final Map<String, String> environmentVariables;
@Nonnull private final String[] excludedEnvironmentVariables;
private final boolean debug;
private final int forkCount;
private final boolean reuseForks;
Expand All @@ -73,6 +74,7 @@ protected DefaultForkConfiguration( @Nonnull Classpath booterClasspath,
@Nonnull Properties modelProperties,
@Nullable String argLine,
@Nonnull Map<String, String> environmentVariables,

This comment has been minimized.

Copy link
@rfscholte

rfscholte Aug 4, 2019

Contributor

This looks like a bad design to me: environmentVariables should contain the effective environmentVariables, so there's no reason to pass excludedEnvironmentVariables, the caller is responsible for that.

This comment has been minimized.

Copy link
@Tibor17

Tibor17 via email Aug 5, 2019

Author Contributor

This comment has been minimized.

Copy link
@Tibor17

Tibor17 Aug 5, 2019

Author Contributor

@rfscholte
I know what you want to say but it would mean to have a structure which selectively inherites only particular environment(s), and adds new ones. Added envs mean the Map<String, String>, removed and inheritted envs mean String[]. Altogether it would mean a complex object in MOJO parameter. Sounds good to me! It is what we call Surefire extension. In this case the old parameter can be marked deprecated.
Is it this what you mean?

This comment has been minimized.

Copy link
@Tibor17

Tibor17 Aug 5, 2019

Author Contributor

We have to decide whether to

  1. deprecate it, introduce a new one (as mentioned above) and remove the old param in 3.0.0, or
  2. keep it going and add more patches.
    I would go for (1).

This comment has been minimized.

Copy link
@rfscholte

rfscholte Aug 6, 2019

Contributor

Having 2 arguments related to environment variables just looks wrong to me. It should be one and I'll leave it up to you ho to solve that: either effective environment variables (the Map<String,String>) or some kind or EnvironmentVariablesResolver, where you can provide several arguments, but which should resolve in effective environment variables

This comment has been minimized.

Copy link
@Tibor17

Tibor17 Aug 6, 2019

Author Contributor

@rfscholte
I think we are speaking the same language! I also want to pass effective environment to the CLI and here are two typical use cases in my POM. Tell me what you think of it from the user perspective. Thx in advance!

  1. Selected only PATH from system env and added MAVEN_OPTS.
<environmentVariables>
    <includedSystemEnvironments>
        <includedSystemEnvironment>PATH</includedSystemEnvironment>
    </includedSystemEnvironments>
    <additionalEnvironments>
        <MAVEN_OPTS>-Xmx512m</MAVEN_OPTS>
    </additionalEnvironments>
</environmentVariables>
  1. In the second use case the user selected all system environment but JAVA_TOOL_OPTIONS. Additionally added NEW_KEY (overrides NEW_KEY in system env if exists).
<environmentVariables>
    <excludedSystemEnvironments>
        <excludedSystemEnvironment>JAVA_TOOL_OPTIONS</excludedSystemEnvironment>
    </excludedSystemEnvironments>
    <additionalEnvironments>
        <NEW_KEY>VAL</NEW_KEY>
    </additionalEnvironments>
</environmentVariables>

This comment has been minimized.

Copy link
@rfscholte

rfscholte Aug 6, 2019

Contributor

using includes/excludes should be enough, it is short and not redundant. additional seems good.
Just wondering what the default behavior should be for <environmentVariables/> aka empty and unspecified aka null.
My first thought: not specifying it means no environment variables, empty would mean include all, exclude nothing.

This comment has been minimized.

Copy link
@Tibor17

Tibor17 Aug 6, 2019

Author Contributor

My first thought: not specifying it means no environment variables, empty would mean include all, exclude nothing.

exactly, empty or not specitied ebvironment means all, which is of course what we call the default behavior or in another words a minimum requirement configuration.

This comment has been minimized.

Copy link
@rfscholte

rfscholte Aug 6, 2019

Contributor

No, that's not what I'm saying. Here are both examples.

<configuration/> <!--  no environment variables in configuration, so `none` environment variables to pass -->

<configuration>
  <environmentVariables/> <!-- specified, so include `all` -->
</configuration>

This will make it very easy to include all or none. I'm just not sure what the impact will be if by default non are passed.

This comment has been minimized.

Copy link
@Tibor17

Tibor17 Aug 6, 2019

Author Contributor

@rfscholte
yes, i got your point before as well.
Currently <environmentVariables/> (and later in this feature) it represents empty Map with additive environment.
In Math form it would mean this:
environmentVariables = [:] // Collections.emptyMap()
effectiveEnvironment = systemEnvironment + environmentVariables where environmentVariables = size of 0 =>
effectiveEnvironment = systemEnvironment
Did I answer your question?
This is backwards compatible.
btw, I would name the parameter by other token environment and not the old environmentVariables but that's the detail.

This comment has been minimized.

Copy link
@Tibor17

Tibor17 Aug 6, 2019

Author Contributor

systemEnvironment is nothing but System.getenv(), nothing less, nothing more!

This comment has been minimized.

Copy link
@Tibor17

Tibor17 Aug 6, 2019

Author Contributor

@rfscholte
Robert, you probably haven't noticed that currently our users rely on the fact that both configurations are equal and system environment of parent process inherited in child process. That's the fact! We cannot change this; otherwise you break their experiences:
<configuration/>
and

<configuration>
  <environmentVariables/>
</configuration>

This comment has been minimized.

Copy link
@rfscholte

rfscholte Aug 6, 2019

Contributor

As said, I don't know what the default behavior should be: all or nothing. All I'm showing is an example based on XML which makes sense if default would be none. If all is the default, consider an option where none are included.

This comment has been minimized.

Copy link
@Tibor17

Tibor17 Aug 6, 2019

Author Contributor

Definitely default should be all.
and both <configuration/> and <environmentVariables/> is ``all.
If I want to have empty environment (silly a bit but anyway possible), then I would do this in my POM:

<environmentVariables>
    <excludedSystemEnvironments/>
</environmentVariables>

I hope it is technicaly possible but we will have tests.

This comment has been minimized.

Copy link
@eolivelli

eolivelli Aug 7, 2019

Contributor

Is it really possible?
I guess we can't strip away all of the env variables, LANG, LC..

This comment has been minimized.

Copy link
@michael-o

michael-o Aug 7, 2019

Member

I would not expect the subenv to be naked because this is not default behavior on Windows and Unix(-like). The default behavior is use as-is, alternatively modify.

This comment has been minimized.

Copy link
@Tibor17

Tibor17 Aug 7, 2019

Author Contributor

@eolivelli
@michael-o
We are talking about Java API and the env passed to the subprocess Runtime.exec(cli, env[]).
I made a test on Windows and Linux too. As I told you before when we had the problem with the build process, I was able to remove single environment and this way pass a new set to the process. I also said to Robert that our users want to unset environment variable(s) https://issues.apache.org/jira/browse/SUREFIRE-963?focusedCommentId=16744241&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-16744241

This comment has been minimized.

Copy link
@Tibor17

Tibor17 Aug 7, 2019

Author Contributor

In Unix world the only exported environment is inherited. Not the local variables.

This comment has been minimized.

Copy link
@Tibor17

Tibor17 Aug 7, 2019

Author Contributor

@eolivelli
I also think we cannot strip away all environment but we have talked with Robert mainly about semantics of tags in POM and the composition of effective environment variables. In the JavaDoc we should clearly state that we do not take any responsibility for user's platform behavior and JDK. All we do, we call Runtime.exec(cli, env[]). The user has to verify (by printing System.getenv()) that the configuration and platform work well for her/him.

This comment has been minimized.

Copy link
@eolivelli

eolivelli Aug 7, 2019

Contributor

Sounds reasonable to me.
We should document it very clearly in the javadocs and on the website.

Thank you for moving this stuff forward

This comment has been minimized.

Copy link
@Tibor17

Tibor17 Aug 8, 2019

Author Contributor

@eolivelli
I would open a PR. That's better view for everyone.

@Nonnull String[] excludedEnvironmentVariables,
boolean debug,
int forkCount,
boolean reuseForks,
Expand All @@ -86,6 +88,7 @@ protected DefaultForkConfiguration( @Nonnull Classpath booterClasspath,
this.modelProperties = modelProperties;
this.argLine = argLine;
this.environmentVariables = toImmutable( environmentVariables );
this.excludedEnvironmentVariables = excludedEnvironmentVariables;
this.debug = debug;
this.forkCount = forkCount;
this.reuseForks = reuseForks;
Expand Down Expand Up @@ -119,7 +122,8 @@ public OutputStreamFlushableCommandline createCommandLine( @Nonnull StartupConfi
@Nonnull File dumpLogDirectory )
throws SurefireBooterForkException
{
OutputStreamFlushableCommandline cli = new OutputStreamFlushableCommandline();
OutputStreamFlushableCommandline cli =
new OutputStreamFlushableCommandline( getExcludedEnvironmentVariables() );

This comment has been minimized.

Copy link
@michael-o

michael-o Aug 4, 2019

Member

I think that those var names are optional, so they should go via setter and not ctor.


cli.setWorkingDirectory( getWorkingDirectory( forkNumber ).getAbsolutePath() );

Expand Down Expand Up @@ -289,6 +293,13 @@ protected Map<String, String> getEnvironmentVariables()
return environmentVariables;
}

@Nonnull
@Override
protected String[] getExcludedEnvironmentVariables()
{
return excludedEnvironmentVariables;
}

@Override
protected boolean isDebug()
{
Expand Down
Expand Up @@ -45,6 +45,7 @@ public abstract class ForkConfiguration
@Nonnull protected abstract Properties getModelProperties();
@Nullable protected abstract String getArgLine();
@Nonnull protected abstract Map<String, String> getEnvironmentVariables();
@Nonnull protected abstract String[] getExcludedEnvironmentVariables();
protected abstract boolean isDebug();
protected abstract int getForkCount();
protected abstract boolean isReuseForks();
Expand Down
Expand Up @@ -62,12 +62,14 @@ public final class JarManifestForkConfiguration
public JarManifestForkConfiguration( @Nonnull Classpath bootClasspath, @Nonnull File tempDirectory,
@Nullable String debugLine, @Nonnull File workingDirectory,
@Nonnull Properties modelProperties, @Nullable String argLine,
@Nonnull Map<String, String> environmentVariables, boolean debug,
@Nonnull Map<String, String> environmentVariables,
@Nonnull String[] excludedEnvironmentVariables,
boolean debug,
int forkCount, boolean reuseForks, @Nonnull Platform pluginPlatform,
@Nonnull ConsoleLogger log )
{
super( bootClasspath, tempDirectory, debugLine, workingDirectory, modelProperties, argLine,
environmentVariables, debug, forkCount, reuseForks, pluginPlatform, log );
environmentVariables, excludedEnvironmentVariables, debug, forkCount, reuseForks, pluginPlatform, log );
}

@Override
Expand Down
Expand Up @@ -67,14 +67,15 @@ public ModularClasspathForkConfiguration( @Nonnull Classpath bootClasspath,
@Nonnull Properties modelProperties,
@Nullable String argLine,
@Nonnull Map<String, String> environmentVariables,
@Nonnull String[] excludedEnvironmentVariables,
boolean debug,
@Nonnegative int forkCount,
boolean reuseForks,
@Nonnull Platform pluginPlatform,
@Nonnull ConsoleLogger log )
{
super( bootClasspath, tempDirectory, debugLine, workingDirectory, modelProperties, argLine,
environmentVariables, debug, forkCount, reuseForks, pluginPlatform, log );
environmentVariables, excludedEnvironmentVariables, debug, forkCount, reuseForks, pluginPlatform, log );
}

@Override
Expand Down
@@ -0,0 +1,47 @@
package org.apache.maven.plugin.surefire.booterclient.lazytestprovider;

/*
* Licensed to the Apache Software Foundation (ASF) under one
* or more contributor license agreements. See the NOTICE file
* distributed with this work for additional information
* regarding copyright ownership. The ASF licenses this file
* to you under the Apache License, Version 2.0 (the
* "License"); you may not use this file except in compliance
* with the License. You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing,
* software distributed under the License is distributed on an
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
* KIND, either express or implied. See the License for the
* specific language governing permissions and limitations
* under the License.
*/

import java.io.IOException;
import java.io.OutputStream;

/**
* Facade flushing {@link OutputStream} and isolating the stream in client.
*/
final class OutputStreamFlushReceiver
implements FlushReceiver
{
private final OutputStream outputStream;

/**
* Wraps an output stream in order to delegate a flush.
*/
OutputStreamFlushReceiver( OutputStream outputStream )
{
this.outputStream = outputStream;
}

@Override
public void flush()
throws IOException
{
outputStream.flush();
}
}
Expand Up @@ -19,9 +19,13 @@
* under the License.
*/

import java.io.IOException;
import java.io.OutputStream;
import java.util.Collection;
import java.util.Collections;
import java.util.Properties;
import java.util.concurrent.ConcurrentLinkedDeque;

import org.apache.maven.shared.utils.cli.CommandLineException;
import org.apache.maven.shared.utils.cli.CommandLineUtils;
import org.apache.maven.shared.utils.cli.Commandline;

/**
Expand All @@ -35,29 +39,37 @@ public class OutputStreamFlushableCommandline
extends Commandline
implements FlushReceiverProvider
{
private final Collection<String> excludedEnvironmentVariables;
private volatile FlushReceiver flushReceiver;

/**
* Wraps an output stream in order to delegate a flush.
* for testing purposes only
*/
private final class OutputStreamFlushReceiver
implements FlushReceiver
public OutputStreamFlushableCommandline()
{
private final OutputStream outputStream;
this( new String[0] );
}

private OutputStreamFlushReceiver( OutputStream outputStream )
{
this.outputStream = outputStream;
}
public OutputStreamFlushableCommandline( String[] excludedEnvironmentVariables )
{
this.excludedEnvironmentVariables = new ConcurrentLinkedDeque<>();
Collections.addAll( this.excludedEnvironmentVariables, excludedEnvironmentVariables );
}

@Override
public final void addSystemEnvironment()
{
Properties systemEnvVars = CommandLineUtils.getSystemEnvVars();

@Override
public void flush()
throws IOException
for ( String key : systemEnvVars.stringPropertyNames() )
{
outputStream.flush();
if ( !excludedEnvironmentVariables.contains( key ) )
{
addEnvironment( key, systemEnvVars.getProperty( key ) );
}
}
}

private volatile FlushReceiver flushReceiver;

@Override
public Process execute()
throws CommandLineException
Expand All @@ -77,5 +89,4 @@ public FlushReceiver getFlushReceiver()
{
return flushReceiver;
}

}
}

1 comment on commit 065001b

@eolivelli
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Tibor17 can you create a pull request for this environment variables branch ?

Please sign in to comment.