Throw an exception on redundant options #401
Conversation
| import org.junit.runners.Parameterized; | ||
| import org.junit.runners.Parameterized.Parameter; | ||
|
|
||
| @RunWith(Enclosed.class) |
There was a problem hiding this comment.
changed from @RunWith(Junit4.class)
| import org.junit.runners.Parameterized.Parameter; | ||
|
|
||
| @RunWith(Enclosed.class) | ||
| public class EnclosedCompilationTest { |
| @RunWith(JUnit4.class) | ||
| public static class CompilationTest { |
There was a problem hiding this comment.
moved the original test class down a scope
| @RunWith(Parameterized.class) | ||
| public static class RedundantOptionsTest { |
|
Our internal tests don't detect any problems with this. I'm now sending it to someone who knows Compile-Testing better than I do to see what he thinks. |
Hi @cpovirk! any thoughts on this? |
|
Sorry, I looked now and discovered that I'd never sent the internal reviewer the message that I'd typed last week :( I've sent it now. The big question we're discussing now is whether we need the configurability at all: Can we just change the behavior to always reject these options? We could release Compile-Testing soon after, so the release would contain few (if any) other changes. Thus, no one would be forced to upgrade, and if users did see trouble, we could easily follow with a release to roll back the change or add configurability. |
|
@cpovirk sure, we can remove the configurability. But I still wonder whether forbidding users from passing in options that the Javac API itself supports would be a bit too assumptious. I'm still fine with removing configurability for now and waiting to see whether it would cause issues for anyone though. |
|
Hi @cpovirk! any thoughts on my comment above? |
|
Agreed, the options are definitely supported, just apparently only when they're set when the VM is first created or something? My main worry with dropping support would be that someone might have a system that automatically copies the options used from a build and tries to use them with Compile-Testing. And maybe the Compile-Testing run was working before (either because the required I still think that all of that is manageable. Compile-Testing doesn't have too many users, so we should be able to hear of problems before we affect too many people. And hopefully most people can just remove the non-functional options when they hear about them. (Sorry that I've continued to be slow here. Things have gotten unexpectedly busy for me for various reasons.) |
|
no worries @cpovirk , thanks for your response! I will spend some time digging into this and will come back to this thread when I feel more confident about this. have a good day! |
|
Hi @cpovirk! If I understand your last comment correctly, you're okay with dropping support for the invalid options - please correct me if I'm wrong. Is anything else needed on my side on this other than rebasing? thanks! |
|
hi @cpovirk! |
|
I'm sorry again. This is one of those things that needs me to set aside just enough time that I can't squeeze it in while waiting for other things. I'll keep trying to find that time, but I expect to keep you waiting still a while longer :( |
Some context in #399