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

allowing markers in jvm argument to preserve commas #1007

Merged
merged 1 commit into from
Apr 4, 2022

Conversation

plastic-karma
Copy link
Contributor

Introducing {} as markers to preserve commas in jvm arguments.

This fixes #675.

Testing

  • added test cases
  • ran pitest cli with --jvmArgs -{agentlib:jdwp=transport=dt_socket,server=y,suspend=y,address=*:5005} which lead to the child JVM waiting until a debugger was connected. Before this change, this failed.

@hcoles
Copy link
Owner

hcoles commented Apr 1, 2022

Thanks @plastic-karma, I take it there was an issue with introducing a 2nd parameter?

@plastic-karma
Copy link
Contributor Author

Thanks @plastic-karma, I take it there was an issue with introducing a 2nd parameter?

Actually no. Here is the trade off that I made:

Introduce new parameter: totally backwards compatible, but need to make changes in up streams like Grade, Ant and Maven

Alter existing parameter: minimally backwards incompatible (if someone uses brackets and commas in their arguments today it might break) but no upstream changes.

I opted to keep changes local. But I am curious what you think @hcoles - we can also introduce a new parameters and I can make changes in the upstreams.

@hcoles
Copy link
Owner

hcoles commented Apr 4, 2022

The escaping is a bit of a hack, but I think it's a reasonable tradeoff.

Thanks for the PR, merging now.

@hcoles hcoles merged commit 874ad99 into hcoles:master Apr 4, 2022
{ Arrays.asList("--jvmArgs", "{a,b,c,d}"), Arrays.asList("a,b,c,d") },
{ Arrays.asList("--jvmArgs", "{a;b;c;d}"), Arrays.asList("{a;b;c;d}") }, // no separators so region markers will not be removed
{ Arrays.asList("--jvmArgs", "{a,b},{c,d},{e,f}"), Arrays.asList("a,b","c,d","e,f") },
{ Arrays.asList("--jvmArgs", "{a,b@c,d}"), Arrays.asList("a,b@c,d") }, // pre-existing '@' will not be replaced with ","
Copy link
Contributor

@szpak szpak Apr 4, 2022

Choose a reason for hiding this comment

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

Is it possible to quote the { usage inside the command?

E.g. {a,{{b}},c}a,{b},c. Just for the future usage. I don't know any existing case for now, probably it is not worth to complicate the parsing process. Maybe it is even forbidden in the JVM arguments? 🤔

Copy link
Owner

Choose a reason for hiding this comment

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

That hasn't been implemented. I agree it would be nice to have, but I think we can probably wait until someone encounters a scenario where it causes a problem.

As far as I'm aware '{' are valid (although I've not turned up anything to confirm/deny this with a quick google). Don't think I've ever encountered it in practice though.

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

Successfully merging this pull request may close these issues.

jvmArgs comma separation breaks for certain JVM arguments
3 participants