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

AgentOptions fails to parse options where destfile contains a comma #358

Closed
jochenberger opened this Issue Nov 10, 2015 · 11 comments

Comments

Projects
None yet
4 participants
@jochenberger
Contributor

jochenberger commented Nov 10, 2015

Caused by: java.lang.IllegalArgumentException: Invalid agent option syntax "destfile=build/jacoco/foo, bar.exec,append=true,dumponexit=true,output=file,jmx=false".
    at org.jacoco.agent.rt.internal_932a715.core.runtime.AgentOptions.<init>(AgentOptions.java:192)
    at org.jacoco.agent.rt.internal_932a715.PreMain.premain(PreMain.java:43)
@marchof

This comment has been minimized.

Show comment
Hide comment
@marchof

marchof Nov 10, 2015

Member

@jochenberger Well, this is a limitation of the current agent options syntax. Any ideas for a syntax extension to properly handle such cases?

Member

marchof commented Nov 10, 2015

@jochenberger Well, this is a limitation of the current agent options syntax. Any ideas for a syntax extension to properly handle such cases?

@jochenberger

This comment has been minimized.

Show comment
Hide comment
@jochenberger

jochenberger Nov 11, 2015

Contributor

If your keys are always lowercase letters, I suggest a RegExp-based approach

String optionString = "destfile=build/jacoco/foo, bar.exec,append=true,dumponexit=true,output=file,jmx=false"
Pattern p = ~",(?=[a-z]+=)"
String [] items = p.split(optionString)
assert items == ["destfile=build/jacoco/foo, bar.exec", "append=true", "dumponexit=true", "output=file", "jmx=false"]
Contributor

jochenberger commented Nov 11, 2015

If your keys are always lowercase letters, I suggest a RegExp-based approach

String optionString = "destfile=build/jacoco/foo, bar.exec,append=true,dumponexit=true,output=file,jmx=false"
Pattern p = ~",(?=[a-z]+=)"
String [] items = p.split(optionString)
assert items == ["destfile=build/jacoco/foo, bar.exec", "append=true", "dumponexit=true", "output=file", "jmx=false"]
@bjkail

This comment has been minimized.

Show comment
Hide comment
@bjkail

bjkail Nov 11, 2015

Contributor

I'm not convinced it's worth the effort, but you could check if the first key is escaped=true and then parse the option string "manually" (without split) so that '' or whatever can be used as an escape char.

Contributor

bjkail commented Nov 11, 2015

I'm not convinced it's worth the effort, but you could check if the first key is escaped=true and then parse the option string "manually" (without split) so that '' or whatever can be used as an escape char.

@marchof

This comment has been minimized.

Show comment
Hide comment
@marchof

marchof Nov 12, 2015

Member

The agent parameter format has its limitations. Also file names with = characters will fail to parse.

Also built-in agents fail to parse such file names, e.g.

> java -Xrunhprof:file=a,b.txt Example
HPROF ERROR: hprof option error: Unknown option: b.txt (file=a,b.txt) [hprof_init.c:495]

Therefore I tempted to close this as "wontfix". If we have a compelling use case I would rather suggest to offer an alternative way to configure the agent (e.g. property file) which does not have such syntax limitations. For offline instrumentation this is already supported.

Member

marchof commented Nov 12, 2015

The agent parameter format has its limitations. Also file names with = characters will fail to parse.

Also built-in agents fail to parse such file names, e.g.

> java -Xrunhprof:file=a,b.txt Example
HPROF ERROR: hprof option error: Unknown option: b.txt (file=a,b.txt) [hprof_init.c:495]

Therefore I tempted to close this as "wontfix". If we have a compelling use case I would rather suggest to offer an alternative way to configure the agent (e.g. property file) which does not have such syntax limitations. For offline instrumentation this is already supported.

@jochenberger

This comment has been minimized.

Show comment
Hide comment
@jochenberger

jochenberger Nov 12, 2015

Contributor

My suggested fix also works for = in the file name.

Contributor

jochenberger commented Nov 12, 2015

My suggested fix also works for = in the file name.

jochenberger added a commit to jochenberger/jacoco that referenced this issue Nov 12, 2015

jochenberger added a commit to jochenberger/jacoco that referenced this issue Nov 12, 2015

@Godin

This comment has been minimized.

Show comment
Hide comment
@Godin

Godin Nov 12, 2015

Member

I also wonder about strong use case for comma in filename.

Member

Godin commented Nov 12, 2015

I also wonder about strong use case for comma in filename.

@jochenberger

This comment has been minimized.

Show comment
Hide comment
@jochenberger

jochenberger Nov 12, 2015

Contributor

Well, I have a directory that contains a comma. And I wonder why jacoco should explicitly forbid that.

Contributor

jochenberger commented Nov 12, 2015

Well, I have a directory that contains a comma. And I wonder why jacoco should explicitly forbid that.

marchof added a commit that referenced this issue Nov 12, 2015

@marchof

This comment has been minimized.

Show comment
Hide comment
@marchof

marchof Nov 12, 2015

Member

Thanks for contributing this fix! I just merged #359.

Member

marchof commented Nov 12, 2015

Thanks for contributing this fix! I just merged #359.

@marchof marchof closed this Nov 12, 2015

@marchof marchof added this to the 0.7.6 milestone Nov 12, 2015

@Godin

This comment has been minimized.

Show comment
Hide comment
@Godin

Godin Feb 14, 2016

Member

@marchof I'm reopening this ticket, because I had hard time debugging patch for Gradle for new configuration option and this change was the root cause:

$ java -cp . -javaagent:jacocoagent-0.7.5.jar=dumponexit=true,inclNoLocationClasses=true Main
Exception in thread "main" java.lang.reflect.InvocationTargetException
        at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
        at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:57)
        at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
        at java.lang.reflect.Method.invoke(Method.java:606)
        at sun.instrument.InstrumentationImpl.loadClassAndStartAgent(InstrumentationImpl.java:382)
        at sun.instrument.InstrumentationImpl.loadClassAndCallPremain(InstrumentationImpl.java:397)
Caused by: java.lang.IllegalArgumentException: Unknown agent option "inclNoLocationClasses".
        at org.jacoco.agent.rt.internal_b0d6a23.core.runtime.AgentOptions.<init>(AgentOptions.java:210)
        at org.jacoco.agent.rt.internal_b0d6a23.PreMain.premain(PreMain.java:43)
        ... 6 more

$ java -cp . -javaagent:jacocoagent-0.7.6-SNAPSHOT.jar=dumponexit=true,inclNoLocationClasses=true Main
OK

$ java -cp . -javaagent:jacocoagent-0.7.6-SNAPSHOT.jar=dumponexit=true,inclnolocationclasses=true Main
OK

Notice incorrect spelling in second case - instead it should be lowercase inclnolocationclasses.

Member

Godin commented Feb 14, 2016

@marchof I'm reopening this ticket, because I had hard time debugging patch for Gradle for new configuration option and this change was the root cause:

$ java -cp . -javaagent:jacocoagent-0.7.5.jar=dumponexit=true,inclNoLocationClasses=true Main
Exception in thread "main" java.lang.reflect.InvocationTargetException
        at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
        at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:57)
        at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
        at java.lang.reflect.Method.invoke(Method.java:606)
        at sun.instrument.InstrumentationImpl.loadClassAndStartAgent(InstrumentationImpl.java:382)
        at sun.instrument.InstrumentationImpl.loadClassAndCallPremain(InstrumentationImpl.java:397)
Caused by: java.lang.IllegalArgumentException: Unknown agent option "inclNoLocationClasses".
        at org.jacoco.agent.rt.internal_b0d6a23.core.runtime.AgentOptions.<init>(AgentOptions.java:210)
        at org.jacoco.agent.rt.internal_b0d6a23.PreMain.premain(PreMain.java:43)
        ... 6 more

$ java -cp . -javaagent:jacocoagent-0.7.6-SNAPSHOT.jar=dumponexit=true,inclNoLocationClasses=true Main
OK

$ java -cp . -javaagent:jacocoagent-0.7.6-SNAPSHOT.jar=dumponexit=true,inclnolocationclasses=true Main
OK

Notice incorrect spelling in second case - instead it should be lowercase inclnolocationclasses.

@Godin Godin reopened this Feb 14, 2016

@Godin

This comment has been minimized.

Show comment
Hide comment
@Godin

Godin Feb 14, 2016

Member

And same happens with existing options:

$ ls *.exec
zsh: no matches found: *.exec

$ java -cp . -javaagent:jacocoagent-0.7.6-SNAPSHOT.jar=dumponexit=true,destFile=dest.exec Main
OK

$ ls *.exec
jacoco.exec

$ java -cp . -javaagent:jacocoagent-0.7.5.jar=dumponexit=true,destFile=dest.exec Main
Exception in thread "main" java.lang.reflect.InvocationTargetException
        at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
        at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:57)
        at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
        at java.lang.reflect.Method.invoke(Method.java:606)
        at sun.instrument.InstrumentationImpl.loadClassAndStartAgent(InstrumentationImpl.java:382)
        at sun.instrument.InstrumentationImpl.loadClassAndCallPremain(InstrumentationImpl.java:397)
Caused by: java.lang.IllegalArgumentException: Unknown agent option "destFile".
        at org.jacoco.agent.rt.internal_b0d6a23.core.runtime.AgentOptions.<init>(AgentOptions.java:210)
        at org.jacoco.agent.rt.internal_b0d6a23.PreMain.premain(PreMain.java:43)
        ... 6 more
Member

Godin commented Feb 14, 2016

And same happens with existing options:

$ ls *.exec
zsh: no matches found: *.exec

$ java -cp . -javaagent:jacocoagent-0.7.6-SNAPSHOT.jar=dumponexit=true,destFile=dest.exec Main
OK

$ ls *.exec
jacoco.exec

$ java -cp . -javaagent:jacocoagent-0.7.5.jar=dumponexit=true,destFile=dest.exec Main
Exception in thread "main" java.lang.reflect.InvocationTargetException
        at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
        at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:57)
        at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
        at java.lang.reflect.Method.invoke(Method.java:606)
        at sun.instrument.InstrumentationImpl.loadClassAndStartAgent(InstrumentationImpl.java:382)
        at sun.instrument.InstrumentationImpl.loadClassAndCallPremain(InstrumentationImpl.java:397)
Caused by: java.lang.IllegalArgumentException: Unknown agent option "destFile".
        at org.jacoco.agent.rt.internal_b0d6a23.core.runtime.AgentOptions.<init>(AgentOptions.java:210)
        at org.jacoco.agent.rt.internal_b0d6a23.PreMain.premain(PreMain.java:43)
        ... 6 more

marchof added a commit that referenced this issue Feb 15, 2016

GitHub #358: Correct error handling for invalid agent arguments
In case of invalid agent arguments (e.g. usage of upper case letters)
such arguments must be reported properly.
@marchof

This comment has been minimized.

Show comment
Hide comment
@marchof

marchof Feb 15, 2016

Member

@Godin Thanks for discovering this! It's actually a bad regression.
Instead of rolling back we might fix it: see PR #376

Member

marchof commented Feb 15, 2016

@Godin Thanks for discovering this! It's actually a bad regression.
Instead of rolling back we might fix it: see PR #376

Godin added a commit that referenced this issue Feb 17, 2016

GitHub #358: Correct error handling for invalid agent arguments
In case of invalid agent arguments (e.g. usage of upper case letters)
such arguments must be reported properly.

@Godin Godin closed this Feb 17, 2016

@jacoco jacoco locked and limited conversation to collaborators Jan 11, 2017

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.