Bug 401776 - [CBI] too many compile warnings
Summary: [CBI] too many compile warnings
Status: RESOLVED FIXED
Alias: None
Product: Platform
Classification: Eclipse Project
Component: Releng (show other bugs)
Version: 4.3   Edit
Hardware: PC Windows 7
: P2 normal (vote)
Target Milestone: 4.4 M2   Edit
Assignee: David Williams CLA
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on: 415941 415942 415943 415944
Blocks: 372792 393922
  Show dependency tree
 
Reported: 2013-02-26 06:09 EST by Markus Keller CLA
Modified: 2014-03-31 17:57 EDT (History)
5 users (show)

See Also:


Attachments
patch for jdt.ui.tests (755 bytes, patch)
2013-02-28 14:44 EST, David Williams CLA
no flags Details | Diff
summary of "grep" of build.properties for "compilerArgs" (1.60 KB, text/plain)
2013-03-04 10:14 EST, David Williams CLA
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Markus Keller CLA 2013-02-26 06:09:01 EST
The CBI builds produce too many compiler warnings.

For PDE builds, projects could configure compiler options, e.g.:

- /org.eclipse.jdt.junit/build.properties says:
javacWarnings..=-unavoidableGenericProblems

- /org.eclipse.jdt.ui/build.properties says:
javacWarnings..=-raw,-unchecked


Is build.properties not processed any more by CBI builds?
Or do we have to add these options to the pom.xml? How?
Comment 1 Markus Keller CLA 2013-02-26 06:56:01 EST
There are also many warnings about the usage of deprecated code. Those were not present in PDE builds, and they are distracting from real issues.

AFAIK, deprecation warnings are disabled by default in ecj. If you don't find who enabled them (Maven default?), you can also disable them using -warn:-deprecation

Furthermore, the discouraged and forbidden access warnings must be disabled in test projects. AFAIK, they were disabled here:
- /org.eclipse.releng.eclipsebuilder/eclipse/buildConfigs/sdk.tests/build.properties:

compilerArg=-inlineJSR -enableJavadoc -encoding ISO-8859-1 -warn:-discouraged,forbidden,warningToken
Comment 2 David Williams CLA 2013-02-26 15:11:03 EST
From what I know and can see, the "tycho adapter" to compilers has our settings as followings in parent pom. 

In general, it is documented at 
http://www.eclipse.org/tycho/sitedocs/tycho-compiler-plugin/compile-mojo.html
 
          <configuration>
            <showWarnings>true</showWarnings>
            <compilerArguments>
              <inlineJSR/>
              <enableJavadoc/>
              <encoding>${project.build.sourceEncoding}</encoding>
              <proceedOnError/>
              <log>${project.build.directory}/@dot.xml</log>
            </compilerArguments>
          </configuration>

I suspect the first element <showWarnings> can only be true or false, all or nothing, something that would apply to all compilers. Compiler specific options need to go in <compilerArguments> section. So, I will remove the <showWarnings> element completely (default is false), and add a compiler specific option of 

<warn>${jdt.compiler.options}</warn>

in the compiler specific section. And then (syntax may be tricky) add a "global" property of <jdt.compiler.options>:-deprecation</jdt.compiler.options>

Let's give that a try for tonight's I-build, and if that works to stop the deprecation warnings (for all), then we can specify different values in "unit tests", and if that works :) advice others to set how they'd like in their own poms, if the defaults are not suitable. 

Let me know if anyone knows/sees a better way.
Comment 4 David Williams CLA 2013-02-26 17:27:11 EST
No joy, so far. In my local test build, I get following. 

[ERROR] Failed to execute goal org.eclipse.tycho:tycho-compiler-plugin:0.17.0-SNAPSHOT:compile (default-compile) on project org.eclipse.osgi: Fatal error compiling: invalid warning configuration: '-warn' -> [Help 1]
org.apache.maven.lifecycle.LifecycleExecutionException: Failed to execute goal org.eclipse.tycho:tycho-compiler-plugin:0.17.0-SNAPSHOT:compile (default-compile) on project org.eclipse.osgi: Fatal error compiling

I'm guessing the ":-deprecation" string is being "corrupted" in the process? Perhaps can not override the default "warn" element? 


Igor, any advice or solutions? Could this be a "Tycho/Maven bug"?
Comment 5 David Williams CLA 2013-02-26 17:44:07 EST
(In reply to comment #4)

> 
> Igor, any advice or solutions? Could this be a "Tycho/Maven bug"?

Is it "legal" to have both 

<configuration>
            <compilerArgument> ... </compilerArgument>

and    <compilerArguments>.... </compilerArguments>

in the same <configuration>? 

Maybe I could pass the whole argument as a string and avoid "error checking?" 
<compilerArgument>warn:-deprecation</compilerArgument> 

Just trial and error?
Comment 6 David Williams CLA 2013-02-26 18:12:52 EST
via IRC, Thanh suggested using just 


<warn:-deprecation />

and he didn't get the fatal error. So ... maybe it just can't be a "variable"?
Comment 7 Markus Keller CLA 2013-02-27 06:23:39 EST
Here are ecj's command line options:
http://help.eclipse.org/topic/org.eclipse.jdt.doc.user/tasks/task-using_batch_compiler.htm?cp=1_3_8_0

Note that "-deprecation" as a top-level command line argument is equivalent to "-warn:+deprecation", i.e. the former *enables* the deprecation warnings we don't want.

> http://www.eclipse.org/tycho/sitedocs/tycho-compiler-plugin/compile-mojo.html

I can't make much out of this document, since it doesn't actually specify how the optional parameters are supposed to be used in the pom. But the examples in comment 2 and comment 6 look completely crazy. An XML element name can only use a restricted character set, so representing a command line argument as an element (and not as an attribute value or element content) is terrible.

If these examples really work, then please open a blocker against Tycho, so that they can fix the doc and implementation.
Comment 8 David Williams CLA 2013-02-27 17:25:36 EST
> I can't make much out of this document, since it doesn't actually specify
> how the optional parameters are supposed to be used in the pom. But the
> examples in comment 2 and comment 6 look completely crazy. An XML element
> name can only use a restricted character set, so representing a command line
> argument as an element (and not as an attribute value or element content) is
> terrible.
> 
> If these examples really work, then please open a blocker against Tycho, so
> that they can fix the doc and implementation.

Well, 2 is not so bad. :) But, yes, once I pasted suggestion in comment 6, my XML Editor told me "invalid element name" so I knew that wasn't right. 

In case of comment 2, though, I think the problem is that the "maven syntax" 
of something like 
<encoding>${project.build.sourceEncoding}</encoding> 
is that  Maven (and/or, Tycho?) translates this as a property value pair, which ends up being 
-encoding=UTF-8 in our case ... which is correct for JDT. 
In the case of trying <warn> this gets translated as 
-warn=:-deprecation 
which JDT doesn't know what to do with (its expecting just -warn:-deprecation (no equals). 

So, good news is I did find a method that works. That was to "mix"
<compilerArgument>
and 
<compilerArguments>
(That is, those are acceptable to combine, which I asked earlier.) 

But, want to hear the full saga? <showWarnings /> must still be true and must come AFTER the <compilerArguments> or it "overrides" our own -warn! (and pretty sure that's Tycho :) No idea why its still needed, if it must come after, and still must be 'true', but seemed to have to be. Must be doing two things with one tag? 

Short story, I was able to run off deprecation warnings for everyone, using 
this configuration in parent pom: 

          <configuration>
            <compilerArgument>-warn:-deprecation</compilerArgument>
            <compilerArguments>
              <verbose />
              <inlineJSR/>
              <enableJavadoc/>
              <encoding>${project.build.sourceEncoding}</encoding>
              <proceedOnError/>
              <log>${project.build.directory}/@dot.xml</log>
            </compilerArguments>
            <showWarnings>true</showWarnings>
          </configuration>

Thanh, here's where we could use your maven expertise for us novices. Or, maybe others know already. But, it'd help me if you could provide a few example patches of the _minimum_ that component owners would have to specify, and where, such as in their repo pom to apply to all unit tests, for example. (At first I thought I could do this in that tycho builder junit section, but think that's literally just packaging ... everything else is already compiled by then). 

I'm guessing its something like 

      <plugin>
          <groupId>org.eclipse.tycho</groupId>
          <artifactId>tycho-compiler-plugin</artifactId>
          <version>${tycho.version}</version>
          <configuration>                   
              <compilerArgument>-warn:-discouraged,forbidden,warningToken</compilerArgument>
          </configuration>
      </plugin>

But, I'm not sure what to "wrap" that in ... something to convey "compiler phase"? In "plugins management"? I'm assuming (hoping) it just takes a minimal specification and all the rest inherited, but I have no idea what to "wrap" it in. I'm guessing something like 

      <build>
      <pluginManagement>
      <plugins>
      <plugin>
          <groupId>org.eclipse.tycho</groupId>
          <artifactId>tycho-compiler-plugin</artifactId>
          <version>${tycho.version}</version>
          <configuration>
             <compilerArgument>-warn:-discouraged,forbidden,warningToken</compilerArgument>
          </configuration>
      </plugin>
      </plugins>
      </pluginManagement>
      </build>


Tip: Thanh, on IRC the other day you were showing what Tycho prints out as compiler arguments in their log, but I've learned that is a little bogus ... all those "null" values, etc. ... but if you look in the @dot.xml files, at least with <verbose /> now on, the compiler will output exactly what its parameter values were. 

Way too much trial and error! Someone should know the answer to this!
Comment 9 David Williams CLA 2013-02-27 23:23:30 EST
This is the first build with -warn:-deprecation. 

I was hoping it'd have cleaned up the list a bit more than it has. 

Any others I should set in the "global defaults"? At least until we figure out (or, someone tells us) how to set "per project/repo"?
Comment 10 David Williams CLA 2013-02-28 00:26:32 EST
(In reply to comment #9)
> This is the first build with -warn:-deprecation. 
> 
> I was hoping it'd have cleaned up the list a bit more than it has. 
> 
> Any others I should set in the "global defaults"? At least until we figure
> out (or, someone tells us) how to set "per project/repo"?

Apologies, forgot to paste "this"

http://download.eclipse.org/eclipse/downloads/drops4/I20130227-2000/testResults.php
Comment 11 Markus Keller CLA 2013-02-28 06:32:55 EST
> Any others I should set in the "global defaults"? At least until we figure
> out (or, someone tells us) how to set "per project/repo"?

Yes, please set global defaults to "-warn:-deprecation,raw,unchecked" for now.

This will hide all generics-related warnings, which are the majority of remaining warnings. Many projects have not yet been fully converted to Java 5 for various reasons. Some also use "-unavoidableGenericProblems", since they have pre-1.5 dependencies. But "-raw,unchecked" also includes all these cases.
We do lose some warnings for projects that should already be fully-generified, but that's acceptable for now.

The remaining big category would be "-discouraged,forbidden" in test bundles, but it's very important to keep these enabled for production bundles.
Comment 12 Paul Webster CLA 2013-02-28 08:16:15 EST
(In reply to comment #11)
> 
> The remaining big category would be "-discouraged,forbidden" in test
> bundles, but it's very important to keep these enabled for production
> bundles.

For repos with a test/ directory, we can create a test pom that has the common compiler flags and update the test bundles to use that as their parent.  For other repos, the same segment will need to go into each test pom, or a <repo>.test-parent will need to be added that the test bundles can use as a parent with a relativePath (similar to our own eclipse-platform-parent).

PW
Comment 13 Thanh Ha CLA 2013-02-28 09:27:09 EST
(In reply to comment #12)
> (In reply to comment #11)
> > 
> > The remaining big category would be "-discouraged,forbidden" in test
> > bundles, but it's very important to keep these enabled for production
> > bundles.
> 
> For repos with a test/ directory, we can create a test pom that has the
> common compiler flags and update the test bundles to use that as their
> parent.  For other repos, the same segment will need to go into each test
> pom, or a <repo>.test-parent will need to be added that the test bundles can
> use as a parent with a relativePath (similar to our own
> eclipse-platform-parent).
> 

+1

I think this is a good solution for this case. As for the build section I don't think you need the PluginManagement part. I think something similar to this is sufficient.

  <build>
    <plugins>
      <plugin>
        <groupId>org.eclipse.tycho</groupId>
        <artifactId>tycho-compiler-plugin</artifactId>
        <version>${tycho.version}</version>
        <configuration>
          <compilerArgument>-warn:-discouraged,forbidden,warningToken</compilerArgument>
        </configuration>
      </plugin>
    </plugins>
  </build>
Comment 14 David Williams CLA 2013-02-28 10:20:32 EST
(In reply to comment #12)
> (In reply to comment #11)
> > 
> > The remaining big category would be "-discouraged,forbidden" in test
> > bundles, but it's very important to keep these enabled for production
> > bundles.
> 
> For repos with a test/ directory, we can create a test pom that has the
> common compiler flags and update the test bundles to use that as their
> parent.  For other repos, the same segment will need to go into each test
> pom, or a <repo>.test-parent will need to be added that the test bundles can
> use as a parent with a relativePath (similar to our own
> eclipse-platform-parent).
> 
> PW

I don't like the idea of making the build more monolithic, and having bundles have parents outside their normal "maven hierarchy".
Comment 15 David Williams CLA 2013-02-28 10:32:07 EST
(In reply to comment #11)
> > Any others I should set in the "global defaults"? At least until we figure
> > out (or, someone tells us) how to set "per project/repo"?
> 
> Yes, please set global defaults to "-warn:-deprecation,raw,unchecked" for
> now.
> 

I've added raw,unchecked. 

http://git.eclipse.org/c/platform/eclipse.platform.releng.aggregator.git/commit/?id=21d932ac9a25a9684ea78617d10ca1785f2e0ec5


We can try a test build, some time, with one or two of the test bundles specifying their own override, before broadcasting general advice. Any volunteers?
Comment 16 Markus Keller CLA 2013-02-28 10:52:35 EST
> We can try a test build, some time, with one or two of the test bundles
> specifying their own override, before broadcasting general advice. Any
> volunteers?

Yes, we can do this for jdt.ui and ltk test suites. But I still have no clue what I have to add to which pom...
Comment 17 David Williams CLA 2013-02-28 14:44:06 EST
Created attachment 227752 [details]
patch for jdt.ui.tests

This patch should work for jdt.ui.tests. (In my local build, once the "global" ones were changed, "ltk" no longer had any "warnings", so you might want to start with just jdt.ui.tests ... but, up to you.) 

I tested this patch in a "partial local re-build" and the warnings in @dot.xml went from 4000+ to 0 so pretty sure that's it. There were no obvious errors in the build, but .... like a I said, a "local rebuild" with just that change, just checking the @dot.xml file (and, console log file of course; no "ERRORS" there). 

If someone from jdt is around and can commit this in next hour or so, I'd be glad to try another "full, clean, local build" just for more re-assurance that all will be ok at 8 PM ... but, still takes an hour or two (even without "signing") so not sure if our time zones align well for "quick check". 

Notice: maven should inherit all other aspects of the "compiler arguments", but pretty sure additional warnings will not be "tacked on" to current, inherited  warnings. If someone wants to override warnings, they'd have to include all the warnings they want (or, don't want) ... which my patch does, but Thanh's very helpful example in previous comment did not, which is why I call attention to it.
Comment 18 David Williams CLA 2013-02-28 14:55:10 EST
(In reply to comment #14)
> (In reply to comment #12)
> > (In reply to comment #11)
> > > 
> > > The remaining big category would be "-discouraged,forbidden" in test
> > > bundles, but it's very important to keep these enabled for production
> > > bundles.
> > 
> > For repos with a test/ directory, we can create a test pom that has the
> > common compiler flags and update the test bundles to use that as their
> > parent.  For other repos, the same segment will need to go into each test
> > pom, or a <repo>.test-parent will need to be added that the test bundles can
> > use as a parent with a relativePath (similar to our own
> > eclipse-platform-parent).
> > 
> > PW
> 
> I don't like the idea of making the build more monolithic, and having
> bundles have parents outside their normal "maven hierarchy".

I see I mis-read your suggestion. You said "those that already had their tests, in a tests/ directory in their repo", and, yes, that is the correct "maven way". On first reading I thought you meant create ONE test directory under our "tycho builder" folder and have everyone point to that. Apologies for reading too fast. 

I actually looked around for a "tests" folder, such as under JDT, and others, I thought most projects had one ... but ... I could not find one in the few that I looked at. 

Just wanted to be clear I don't object to projects using a "test parent" if they already have things set up that way. 

Not sure if/how this effects Maven's ability to "automatically, incrementally test what it just built"  ability ... but ... we are a long way from using that anyway, so would not be a factor.
Comment 19 Markus Keller CLA 2013-02-28 15:44:54 EST
(In reply to comment #17)
> Created attachment 227752 [details] [diff]
> patch for jdt.ui.tests

Thanks a lot! Released as http://git.eclipse.org/c/jdt/eclipse.jdt.ui.git/commit/?id=77d28d022c5f2dcde11ddf185debfb49b928b91b

Feel free to do a special build or not. Otherwise, we'll see the result tomorrow. I'll check back in the next 2h or so, to see if I have to revert.
Comment 20 David Williams CLA 2013-02-28 17:32:54 EST
(In reply to comment #19)
> (In reply to comment #17)
> > Created attachment 227752 [details] [diff]
> > patch for jdt.ui.tests
> 
> Thanks a lot! Released as
> http://git.eclipse.org/c/jdt/eclipse.jdt.ui.git/commit/
> ?id=77d28d022c5f2dcde11ddf185debfb49b928b91b
> 
> Feel free to do a special build or not. Otherwise, we'll see the result
> tomorrow. I'll check back in the next 2h or so, to see if I have to revert.

My "special build" went fine, sdk's same size, no errors in log, generated 390 class files.  
And, it even got "jdt.ui.tests" off the list of discouraged access! :) 

More important (to me :), looking at the @dot.xml file, it did inherit all the other compiler options we wanted it to inherit ... encoding is so nice its listed twice :) ... not sure why that is? But, that was one thing important to confirm. Thanks for the "early test case".

        ...
        <argument value="-encoding"/>
        <argument value="UTF-8"/>
        <argument value="-enableJavadoc"/>
        <argument value="-encoding"/>
        <argument value="UTF-8"/>
        <argument value="-inlineJSR"/>
        <argument value="-log"/>
        <argument value="/data/shared/eclipse/builds/4I/master/gitCache/eclipse.platform.releng.aggregator/eclipse.jdt.ui/org.eclipse.jdt.ui.tests/target/@dot.xml"/>
        <argument value="-proceedOnError"/>
        <argument value="-verbose"/>
        <argument value="-warn:-deprecation,raw,unchecked,discouraged,forbidden,warningToken"/>
</command_line>
Comment 21 David Williams CLA 2013-02-28 17:43:25 EST
And, for the curious, it appears there are 5 repos (of 24) with 'tests' directories. None with existing pom.xml files. 

$ find ./ -maxdepth 2 -name tests -type d
./eclipse.platform.runtime/tests
./eclipse.platform.swt/tests
./eclipse.platform.resources/tests
./eclipse.platform.team/tests
./eclipse.platform.ui/tests

Not sure which would be easier for those projects ... either way, each bundle's existing pom would have to be changed; either to point to its new parent (a new one, in the /tests directory) or copy/paste the snippet into each existing pom. 

But either is an acceptable option, asfaik.
Comment 22 David Williams CLA 2013-03-01 00:48:25 EST
FWIW, to cross reference, I've opened bug 402086 for Tycho to do this "automatically" ... hope they don't say "its already possible with special flag .. " (well, actually, hope they do! :)
Comment 23 Paul Webster CLA 2013-03-01 07:14:54 EST
(In reply to comment #21)
> $ find ./ -maxdepth 2 -name tests -type d
> ./eclipse.platform.runtime/tests
> ./eclipse.platform.swt/tests
> ./eclipse.platform.resources/tests
> ./eclipse.platform.team/tests
> ./eclipse.platform.ui/tests
> 
> Not sure which would be easier for those projects ... either way, each
> bundle's existing pom would have to be changed; either to point to its new
> parent (a new one, in the /tests directory) or copy/paste the snippet into
> each existing pom. 

For these examples, I'd suggest putting the common configuration in the tests/pom.xml, and then making that the parent for the test bundles.

For repos that don't have the tests directory but have more the a handful of test plugins, an option would be to put the common configuration in a repo/test-parent/pom.xml, and use that as the parent of the test modules (include the relativePath element).  If there are only 2 test bundles, then it might not be worth the effort.

PW
Comment 24 Markus Keller CLA 2013-03-01 08:08:09 EST
(In reply to comment #20)
> encoding is so nice its listed twice :) ... not sure why that is?

I also don't know why, but it was already the case in I20130226-0800.


I compared I20130228-2000 (latest CBI build) to I20130219-1600 (latest PDE build), and found one missing piece in CBI:

Discouraged Access Warnings in org.eclipse.ant.launching:
http://download.eclipse.org/eclipse/downloads/drops4/I20130219-1600/compilelogs/plugins/org.eclipse.ant.launching_1.0.300.v20130206-163325/@dot.html
=> Note that the monster mb060_run-maven-build_output.txt does contain the compile warning (grep for "AntLaunchingUtil").
=> It looks like not all compiler outputs are copied to the results page.

Other than that, the compile warnings look reasonable, and the discouraged access warnings on org.eclipse.jdt.ui.tests are gone. We just have to find a way to inject something like attachment 227752 [details] into all test bundles in a generic way.
Comment 25 David Williams CLA 2013-03-04 10:00:01 EST
(In reply to comment #24)
> (In reply to comment #20)
> I compared I20130228-2000 (latest CBI build) to I20130219-1600 (latest PDE
> build), and found one missing piece in CBI:
> 

I reopened bug 400753 as suspect there's some general condition where we miss warnings and/or compile errors that deserves its own investigation.
Comment 26 David Williams CLA 2013-03-04 10:09:39 EST
(In reply to comment #24)
> (In reply to comment #20)

> We just have to find a
> way to inject something like attachment 227752 [details] into all test
> bundles in a generic way.

I don't think there's much to automatically do here (nothing that's going to happen any time soon). And, I think that its already generic enough, that committers can go through and copy/paste the following, into all their tests bundle pom.xml files, between what should be the last two tags in their current pom.xml files. (and if anyone has anything more complicated, so that its confusing, to call those out specifically for extra help. 

 <packaging>eclipse-test-plugin</packaging>
<!-- insert here -->
 </project>

= = = = the generic copy/paste part = = = =

  <build>
    <plugins>
      <plugin>
        <groupId>org.eclipse.tycho</groupId>
        <artifactId>tycho-compiler-plugin</artifactId>
        <version>${tycho.version}</version>
        <configuration>
          <compilerArgument>-warn:-deprecation,raw,unchecked,discouraged,forbidden,warningToken</compilerArgument>
        </configuration>
      </plugin>
    </plugins>
  </build>

= = = = = = = =
Comment 27 David Williams CLA 2013-03-04 10:14:24 EST
Created attachment 227881 [details]
summary of "grep" of build.properties for "compilerArgs"

I'm pretty sure I have most repositories/projects loaded in one workspace/directory and did a grep to find existing cases of "custom" compilerArgs

grep -r --include build.properties compilerArg *

Besides tests, I think there was only one case of someone having something special, 
eclipse.jdt.core/org.eclipse.jdt.compiler.apt.tests/build.properties:compilerArg=-proc:none

Not sure that has to do with warnings, but suggest a special "override"? 

See attachment for more details.
Comment 28 David Williams CLA 2013-03-04 10:31:54 EST
(In reply to comment #27)
> Created attachment 227881 [details]

> eclipse.jdt.core/org.eclipse.jdt.compiler.apt.tests/build.properties:
> compilerArg=-proc:none
> 
> Not sure that has to do with warnings, but suggest a special "override"? 

I've opened bug 402344 to cover this "special case". 

But think all other cases of "test overrides" can use this bug to commit the generic change .... and if anyone has a non-generic pom that looks more complicated, open a new bug and we'll take a look.
Comment 29 Markus Keller CLA 2013-03-04 13:31:28 EST
(In reply to comment #26)
Replacing a single line in build.properties with a dozen pom.xml lines is not a beauty. And it would still be good to have a common set of 2nd class warnings that are disabled by default for tests, so that test bundles that don't need more options can just go with the defaults. And so that we have a common place to add new defaults for tests in the future.

We have roughly one test bundle per production bundle (e.g. in the eclipse.platform.text repo), so I'd rather go with something like Paul suggested in comment 23.

(In reply to comment #27)
Most build.properties don't use the "compilerArg" property to specify additional warnings to suppress, but "javacWarnings.." or "javacWarnings.<jarName>.jar".
Comment 30 Dani Megert CLA 2013-03-15 04:05:31 EDT
At this point, the access warnings on the tests annoy me most.
Comment 31 Paul Webster CLA 2013-03-15 08:01:04 EDT
Platform UI modification ready to go in M7: https://git.eclipse.org/r/#/c/11192/

PW
Comment 32 David Williams CLA 2013-03-30 16:45:16 EDT
I noticed in recent Tycho documentation, see 

http://wiki.eclipse.org/Tycho/FAQ#How_to_configure_warning.2Ferror_settings_of_the_OSGi_compiler.3F

That they (presumably) allow you to specify per-project settings with 

<compilerArguments>
<properties>${project.basedir}/.settings/org.eclipse.jdt.core.prefs</properties>
</compilerArguments>

I tried setting this "globally" (in my local test builds) and found that Tycho will then "fail" if this file does not exist at all for a project. I opened bug 404633 for that. 

I discovered, one by one, 4 or 5 project that did not have any .settings directory (and of course no .settings/org.eclipse.jdt.core.prefs file). I fixed these "locally" to try and test this method of getting per, project settings, but then started running in to fatal? JavaDoc errors? Not sure what that was about, but might have simply been due to my "quick fixes" (e.g. I "fixed" some by if .settings directory existed, I "touched" org.eclipse.jdt.core.prefs so if it already existed, it'd just get new date, but if did not exist would create an empty file ... probably can't use empty settings? 

But, not sure if its any better/easier to try and get everyone to have accurate org.eclipse.jdt.core.prefs files ... or ... limp along as we are? I mean, I like the idea of it ... they should be accurate, and exist! ... but ... might be a lot of work (and, not to sound too paranoid, but sounds like a new Tycho function that might cause other, unexpected issues?) And in principle, I guess, some committers may like having one set of settings for their "workspace project", but another set of settings for the actual build? (not sure how platform commmitters work). 

Anyone have any thoughts/advice? Or, small projects to test it out on?
Comment 33 David Williams CLA 2013-03-30 16:48:01 EDT
Oh, and, wanted to remind, that we are still using the "global" setting of 

<compilerArgument>-warn:-deprecation,raw,unchecked</compilerArgument>

which ... is probably not ideal ... but ... may live with for Kepler? 

If anyone feels strongly about it, now's the time to speak up, so there would be some weeks before M7 for committers to fix their projects.
Comment 34 Dani Megert CLA 2013-04-02 10:50:06 EDT
(In reply to comment #32)
> I noticed in recent Tycho documentation, see 
> 
> http://wiki.eclipse.org/Tycho/FAQ#How_to_configure_warning.
> 2Ferror_settings_of_the_OSGi_compiler.3F
> 
> That they (presumably) allow you to specify per-project settings with 
> 
> <compilerArguments>
> <properties>${project.basedir}/.settings/org.eclipse.jdt.core.prefs</
> properties>
> </compilerArguments>

This is not what we want. The global builder must define how/what problems are reported. Projects can then ignore some of them if really needed. Project specific settings are often more strict or less strict than what we want to see in the global/common report, e.g. a project might enable to report Javadoc warnings in the IDE, but they should not appear in the global report.
Comment 35 Paul Webster CLA 2013-04-09 09:32:11 EDT
(In reply to comment #31)
> Platform UI modification ready to go in M7:
> https://git.eclipse.org/r/#/c/11192/

Thanh, could you review the pom changes I made?

PW
Comment 36 David Williams CLA 2013-04-29 07:09:59 EDT
I think this bug should be resolved, as fixed, for M7, from RELENG component point of view. Given the limitations of Tycho/Maven builder, if individual components want to limit their warnings, then they should open their own bugs, and make appropriate changes to their own POMs as discussed. 

Am I missing anything? Anything more "releng" should do?
Comment 37 Dani Megert CLA 2013-04-29 08:58:29 EDT
(In reply to comment #36)
> I think this bug should be resolved, as fixed, for M7, from RELENG component
> point of view. Given the limitations of Tycho/Maven builder, if individual
> components want to limit their warnings, then they should open their own
> bugs, and make appropriate changes to their own POMs as discussed. 
> 
> Am I missing anything? Anything more "releng" should do?

The builder should ignore those warnings for test project by default to reduce duplicate entries in all the test pom files.
Comment 38 David Williams CLA 2013-04-29 09:06:49 EDT
(In reply to comment #37)
> (In reply to comment #36)
> > I think this bug should be resolved, as fixed, for M7, from RELENG component
> > point of view. Given the limitations of Tycho/Maven builder, if individual
> > components want to limit their warnings, then they should open their own
> > bugs, and make appropriate changes to their own POMs as discussed. 
> > 
> > Am I missing anything? Anything more "releng" should do?
> 
> The builder should ignore those warnings for test project by default to
> reduce duplicate entries in all the test pom files.

Which warnings? 
Our builder already sets
<compilerArgument>-warn:-deprecation,raw,unchecked</compilerArgument>

If you are saying this releng component should work on fixing Tycho ... then ... I think you are asking for too much :)
Comment 39 David Williams CLA 2013-04-29 09:13:39 EDT
(In reply to comment #38)
> (In reply to comment #37)
> > (In reply to comment #36)
> > > I think this bug should be resolved, as fixed, for M7, from RELENG component
> > > point of view. Given the limitations of Tycho/Maven builder, if individual
> > > components want to limit their warnings, then they should open their own
> > > bugs, and make appropriate changes to their own POMs as discussed. 
> > > 
> > > Am I missing anything? Anything more "releng" should do?
> > 
> > The builder should ignore those warnings for test project by default to
> > reduce duplicate entries in all the test pom files.
> 
> Which warnings? 
> Our builder already sets
> <compilerArgument>-warn:-deprecation,raw,unchecked</compilerArgument>
> 
> If you are saying this releng component should work on fixing Tycho ... then
> ... I think you are asking for too much :)

Perhaps you forgot/missed this bug open on Tycho ... I think that's where the fix should be: bug 402086.
Comment 40 Markus Keller CLA 2013-04-29 09:52:51 EDT
> Perhaps you forgot/missed this bug open on Tycho ... I think that's where
> the fix should be: bug 402086.

That would also be nice to have, but it doesn't fix the main problem:

Test projects still have to duplicate all the common compiler options that should be used for all tests in the SDK (comment 1). Duplicated options are a maintenance hell, which cannot be the goal of the CBI.

The goal is that all bundles that specify
    <packaging>eclipse-test-plugin</packaging>
in their pom get a default set of compiler arguments for free.

If that's not feasible, can we at least have a global property somewhere that defines the default <compilerArgument> options, so that projects that copy-paste comment 26 can refer to that global property, instead of having to list all arguments?
Comment 41 David Williams CLA 2013-04-29 10:13:11 EDT
(In reply to comment #40)
> > Perhaps you forgot/missed this bug open on Tycho ... I think that's where
> > the fix should be: bug 402086.
> 
> That would also be nice to have, but it doesn't fix the main problem:
> 
> Test projects still have to duplicate all the common compiler options that
> should be used for all tests in the SDK (comment 1). Duplicated options are
> a maintenance hell, which cannot be the goal of the CBI.
> 
> The goal is that all bundles that specify
>     <packaging>eclipse-test-plugin</packaging>
> in their pom get a default set of compiler arguments for free.
> 
> If that's not feasible, can we at least have a global property somewhere
> that defines the default <compilerArgument> options, so that projects that
> copy-paste comment 26 can refer to that global property, instead of having
> to list all arguments?

Ok, I think I might be beginning to get a better idea of what is being asked for, but let's review. 

Currently everyone does get 
<compilerArgument>-warn:-deprecation,raw,unchecked</compilerArgument> 
"for free", unless they have their own. 

But, that's for all bundles, I think you are saying there should be a separate set, specifically for test bundles?  

I do not know how to do that, though there could be a way? 

But if would be relatively easy to define a variable, say 
<testCompilerArguments>-warn:-deprecation,raw,unchecked</testCompilerArgument>
and then each component, when they copy/paste the section in their POM could say 
<compilerArgument>${testCompilerArguments}</compilerArgument> 
if they wanted the defaults. 

Is that what you'd find helpful? (Doesn't seem that helpful to me :) ... but, glad to add variable to parent POM if you do. 

More important point of (my) confusion ... are you 'expanding' this to mean more than just compiler warnings? Or ALL potential, default, compiler settings? 
What else would vary that is not already being inherited?
Comment 42 Markus Keller CLA 2013-04-29 11:08:37 EDT
> But, that's for all bundles, I think you are saying there should be a
> separate set, specifically for test bundles?  

Yes, that's what we had with PDE build. But I also don't speak enough Maven to know how to do that or to say it's impossible.

> <compilerArgument>${testCompilerArguments}</compilerArgument> 
> [..]
> Is that what you'd find helpful? (Doesn't seem that helpful to me :) ...
> but, glad to add variable to parent POM if you do. 

Yes, that would be very helpful. It would at least allow us to apply [DRY] to our data, given that Maven/Tycho and the POM format don't seem to subscribe to basic software construction principles...

If you define ${defaultCompilerArguments} as well, then bundles with special needs can also reuse that when they add more arguments.

I think a single mechanism for adding compiler arguments is enough for now. PDE build had separate javacWarnings.<library> and javaErrors.<library> options per library and a compilerArg option per project, but I don't know how that would translate to Maven.

[DRY] http://en.wikipedia.org/wiki/Don%27t_repeat_yourself
Comment 43 David Williams CLA 2013-05-01 15:44:11 EDT
I'll look at adding variables soon after M7, if that helps.
Comment 44 David Williams CLA 2013-05-17 17:41:19 EDT
Obviously not RC1, but I've not given up on 4.3 ... though, only if I can find a simple/safe way to do it that makes a big difference. I think the "variables" idea is fine, but I'd hate to have to see all components have to use/react to that this late in the game. But ... I am starting learn a little bit of Maven. :)
Comment 45 David Williams CLA 2013-05-27 23:32:29 EDT
I've not had time to look at this. I suggest we target for Luna. 
If we find a good, clean solution, then consider backporting to SR1, but ... still don't know what the best solution is.
Comment 46 Dani Megert CLA 2013-08-08 05:36:06 EDT
Still too much noise on the results page. Can we resurrect this? 

Another idea (not sure this can be done with Tycho): could we introduce/add a special parent pom between the (test) bundle's pom and the repo root pom? This would allow us to set the (test) compiler settings only once per repo.
Comment 47 Thanh Ha CLA 2013-08-08 09:46:00 EDT
(In reply to comment #46)
> Still too much noise on the results page. Can we resurrect this? 
> 
> Another idea (not sure this can be done with Tycho): could we introduce/add
> a special parent pom between the (test) bundle's pom and the repo root pom?
> This would allow us to set the (test) compiler settings only once per repo.

It is possible to override Maven settings for a plugin from a parent pom with another parent pom. If all of the tests need the same settings that are different from the regular settings perhaps it's worth creating a parent pom just for tests?
Comment 48 Dani Megert CLA 2013-08-08 09:51:20 EDT
(In reply to comment #47)
> (In reply to comment #46)
> > Still too much noise on the results page. Can we resurrect this? 
> > 
> > Another idea (not sure this can be done with Tycho): could we introduce/add
> > a special parent pom between the (test) bundle's pom and the repo root pom?
> > This would allow us to set the (test) compiler settings only once per repo.
> 
> It is possible to override Maven settings for a plugin from a parent pom
> with another parent pom. If all of the tests need the same settings that are
> different from the regular settings perhaps it's worth creating a parent pom
> just for tests?

Right. Preferably at the top, so that we don't have to do it in every repository. On the other hand, such an injected pom could also be interesting for a single repository (not related to tests). How would I do this (e.g. to set specific settings for all my JDT projects)?
Comment 49 Thanh Ha CLA 2013-08-08 10:10:11 EDT
(In reply to comment #48)
> Right. Preferably at the top, so that we don't have to do it in every
> repository. On the other hand, such an injected pom could also be
> interesting for a single repository (not related to tests). How would I do
> this (e.g. to set specific settings for all my JDT projects)?

Using JDT Core as an example. If your injected settings should be used for all projects in the repo you could reuse the root pom.xml [1] since all of them already depend on it as the parent. 

I believe you should be able to redeclare the plugins you want to change and it'll inherit values from it's parent and override values you've configured. So if you want  to change compiler settings you should be able to add this section to the root pom.xml [1] and modify the values you'd like different.

    <build>
      <pluginManagement>
        <plugin>
          <groupId>org.eclipse.tycho</groupId>
          <artifactId>tycho-compiler-plugin</artifactId>
          <version>${tycho.version}</version>
          <dependencies>
            <dependency>
              <groupId>org.eclipse.jdt</groupId>
              <artifactId>org.eclipse.jdt.core</artifactId>
              <version>${cbi-jdt-version}</version>
            </dependency>
          </dependencies>
          <configuration>
            <compilerArgument>-warn:-deprecation,raw,unchecked</compilerArgument>
            <compilerArguments>
              <verbose/>
              <inlineJSR/>
              <enableJavadoc/>
              <encoding>${project.build.sourceEncoding}</encoding>
              <proceedOnError/>
              <log>${project.build.directory}/@dot.xml</log>
            </compilerArguments>
            <showWarnings>true</showWarnings>
            <excludeResources>
              <exclude>**/package.html</exclude>
            </excludeResources>
          </configuration>
        </plugin>
      </pluginManagement>
    </build>



If however you'd like some projects to be different and the remaining ones to continue to inherit from the original parent you could create a new parent pom in the repo by making a directory and adding a pom.xml there. Have have the new pom's parent be the root pom.xml and have all the projects you want to use this as the parent change their parents to the new pom.xml.

Example new repo parent pom:

<project xmlns="http://maven.apache.org/POM/4.0.0" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd">
  <modelVersion>4.0.0</modelVersion>

  <parent>
    <groupId>eclipse.jdt.core</groupId>
    <artifactId>eclipse.jdt.core</artifactId>
    <version>4.4.0-SNAPSHOT</version>
  </parent>

  <groupId>eclipse.jdt.core</groupId>
  <artifactId>jdt-core-tests-parent</artifactId>
  <version>4.4.0-SNAPSHOT</version>
  <packaging>pom</packaging>

  <build>
    <pluginManagement>
      <plugin>
        <groupId>org.eclipse.tycho</groupId>
        <artifactId>tycho-compiler-plugin</artifactId>

         ...

      </plugin>
    </pluginManagement>
  </build>
</project>


[1] http://git.eclipse.org/c/jdt/eclipse.jdt.core.git/tree/pom.xml
Comment 50 Thanh Ha CLA 2013-08-08 10:14:04 EDT
As I understand it, how pom inheritence works is most specific will override settings. If you don't explicitly declare a setting it'll inherit that setting from the parent pom above it. You can have a chain of poms inheriting downwards.

At the moment most bundles in the platform depend on at least 2 parents above it.


eclipse-platform-parent pom > repo's root pom > bundle pom
Comment 51 Dani Megert CLA 2013-08-21 10:25:20 EDT
(In reply to comment #49)

Thanks Thanh. The latter approach with a new parent pom is what I'm looking for. 

Assuming the new pom is in a new 'tests' folder i.e. we'd have
repo-root/test/pom.xml. How would I point to that new parent pom from e.g. the jdt.core plug-in pom.xml?
Comment 52 David Williams CLA 2013-08-21 10:52:02 EDT
I'd suggest handling this on a repo-by-repo basis ... we need to work towards making our builds more modular, not making them more monolithic. 

(You could still have your own "test parent" ... but other repositories, for example, already have their "tests" broken out in to a separate folder (similar to "bundles" or "features" so I'm not sure there is "one size that fits all).
Comment 53 Dani Megert CLA 2013-08-21 10:53:51 EDT
(In reply to comment #52)
> I'd suggest handling this on a repo-by-repo basis ... we need to work
> towards making our builds more modular, not making them more monolithic. 

That's what we're just discussing in the past view comments ;-).
Comment 54 Dani Megert CLA 2013-08-21 10:54:36 EDT
(In reply to comment #53)
> (In reply to comment #52)
> > I'd suggest handling this on a repo-by-repo basis ... we need to work
> > towards making our builds more modular, not making them more monolithic. 
> 
> That's what we're just discussing in the past view comments ;-).
                                              * few *
Comment 55 David Williams CLA 2013-08-21 11:04:04 EDT
(In reply to comment #54)
> (In reply to comment #53)
> > (In reply to comment #52)
> > > I'd suggest handling this on a repo-by-repo basis ... we need to work
> > > towards making our builds more modular, not making them more monolithic. 
> > 
> > That's what we're just discussing in the past view comments ;-).
>                                               * few *

Ok, sorry, hard for me to keep up. 

In that case, I'd recommend a new "folder" in your repo, say named "tests", as peer of "bundles" and "features", and it in have a folder, say "test-parent", which would have your "test pom" in it. Then your test bundles would name it as their parent ... something similar to ".../tests/test-parent" and that test-parent pom would refer to the repo pom similar to how your bundles currently do "../repo pom" (sorry I can't be more exact right now ... just trying to be a little bit helpful without actually looking at your existing code/repo). 

Let me know if I can help more (later).
Comment 56 Thanh Ha CLA 2013-08-21 11:13:21 EDT
(In reply to comment #51)
> (In reply to comment #49)
> 
> Thanks Thanh. The latter approach with a new parent pom is what I'm looking
> for. 
> 
> Assuming the new pom is in a new 'tests' folder i.e. we'd have
> repo-root/test/pom.xml. How would I point to that new parent pom from e.g.
> the jdt.core plug-in pom.xml?

For example if your bundle was eclipse.jdt.core/org.eclipse.jdt.core.tests.compiler

You will need to update plug-in's pom.xml with the Group, artifact, and version of the new parent and then add an additional parameter <relativePath>.

Eg.
  <parent>
    <artifactId>eclipse.jdt.core.tests-parent</artifactId>
    <groupId>eclipse.jdt.core</groupId>
    <version>4.4.0-SNAPSHOT</version>
    <relativePath>../tests/</relativePath>
  </parent>


(By default relativePath looks up 1 directory so "..")
Comment 57 Dani Megert CLA 2013-08-22 11:55:20 EDT
(In reply to comment #43)
> I'll look at adding variables soon after M7, if that helps.

Do we really need a new variable? Test bundles already declare
<packaging>eclipse-test-plugin</packaging>
Can't we use that information in the eclipse parent pom to make the distinction?


> I'd suggest handling this on a repo-by-repo basis ... 
I've committed a change that goes into that direction (hopefully the build won't break), BUT: this is really not what we need, because we need one place where we can set the compiler arguments for production and test bundles. Currently, we disable raw type warning for all bundles but that's wrong for production bundles.
Comment 58 Dani Megert CLA 2013-08-22 12:03:40 EDT
(In reply to comment #57)
> (In reply to comment #43)
> > I'll look at adding variables soon after M7, if that helps.
> 
> Do we really need a new variable? Test bundles already declare
> <packaging>eclipse-test-plugin</packaging>
> Can't we use that information in the eclipse parent pom to make the
> distinction?
> 
> 
> > I'd suggest handling this on a repo-by-repo basis ... 
> I've committed a change that goes into that direction (hopefully the build
> won't break), BUT: this is really not what we need, because we need one
> place where we can set the compiler arguments for production and test
> bundles. Currently, we disable raw type warning for all bundles but that's
> wrong for production bundles.

Thanh any idea whether we can use <packaging>eclipse-test-plugin</packaging> is the eclipse parent pom to solve the issue, or, how to define a variable that lets us declare whether it's a test or production bundle. And then use that information to set the compiler arguments?
Comment 59 Thanh Ha CLA 2013-08-22 14:00:09 EDT
(In reply to comment #58)

The parent pom does not know the packaging types of poms under it since parent poms are used as metadata for more specific poms under it.

poms inherit configuration from their declared parent pom, whom might have another parent pom which inherits configuration from that all the way up the chain.

I think the best choice here is to have a parent pom for tests which declare the compiler settings that tests need and set this pom as the parent for these test bundles and another parent pom for regular bundles.

If you want to have tests in all repos to inherit the same settings we might consider making a new folder in the aggregator and create a test-parent pom there. Then in each repo also declare a new folder with a pom for tests that inherit the aggregator's test-parent pom.

This way the tests in a repo would depend on it's own repo's test-parent pom and this pom may have it's own override settings if the project wants to fine tune their own compiler setting, or not and simply just inherit the aggregator's test-parent pom settings.


Don't forget that if you want to override settings for a maven plugin in any pom anywhere, all you have to do is redeclare that plugin at a more specific spot such as the repo pom, or even the exact plugin pom. Any configurations you don't specify will be inherited from the parent.
Comment 60 Dani Megert CLA 2013-08-23 04:21:50 EDT
(In reply to comment #59)
> (In reply to comment #58)
> 
> The parent pom does not know the packaging types of poms under it since
> parent poms are used as metadata for more specific poms under it.

I see.


> I think the best choice here is to have a parent pom for tests which declare
> the compiler settings that tests need and set this pom as the parent for
> these test bundles and another parent pom for regular bundles.

I have that working inside one repository (tried with jdt.ui).


> If you want to have tests in all repos to inherit the same settings we might
> consider making a new folder in the aggregator and create a test-parent pom
> there. Then in each repo also declare a new folder with a pom for tests that
> inherit the aggregator's test-parent pom.

Yes, I was thinking along that as well, BUT: how can we share the information which is currently in a repository's root pom (e.g. module info, tycho.scmUrl, ...)? We don't want to duplicate that information. The repo's test/runtime pom would have to be able to inherit its repository info (current repo pom) and specify the different eclipse parent test/runtime pom.
Comment 61 Paul Webster CLA 2013-08-23 08:12:05 EDT
Wouldn't this just be the same pattern we used in eclipse.platform.ui?  The test-parent-pom [1] inherits from the repo pom, and the test poms use the test parent pom [2]

[1] http://git.eclipse.org/c/platform/eclipse.platform.ui.git/tree/tests/pom.xml

[2] http://git.eclipse.org/c/platform/eclipse.platform.ui.git/tree/tests/org.eclipse.ui.tests/pom.xml

PW
Comment 62 Dani Megert CLA 2013-08-23 08:23:05 EDT
(In reply to comment #61)
> Wouldn't this just be the same pattern we used in eclipse.platform.ui?  The
> test-parent-pom [1] inherits from the repo pom, and the test poms use the
> test parent pom [2]
> 
> [1]
> http://git.eclipse.org/c/platform/eclipse.platform.ui.git/tree/tests/pom.xml
> 
> [2]
> http://git.eclipse.org/c/platform/eclipse.platform.ui.git/tree/tests/org.
> eclipse.ui.tests/pom.xml
> 
> PW

No. The test pom still points to the repository pom and not the eclipse parent pom. That's where I'm currently with jdt.ui. But if I do that I will have to copy parts of the repo pom to the test pom and I don't want such duplication.
Comment 63 Paul Webster CLA 2013-08-23 08:25:14 EDT
(In reply to comment #62)
> 
> No. The test pom still points to the repository pom and not the eclipse
> parent pom. That's where I'm currently with jdt.ui. But if I do that I will
> have to copy parts of the repo pom to the test pom and I don't want such
> duplication.

Right, but the repository pom points to the eclipse-parent pom, and so it part of the inheritance chain.

PW
Comment 64 Dani Megert CLA 2013-08-23 08:31:49 EDT
(In reply to comment #63)
> (In reply to comment #62)
> > 
> > No. The test pom still points to the repository pom and not the eclipse
> > parent pom. That's where I'm currently with jdt.ui. But if I do that I will
> > have to copy parts of the repo pom to the test pom and I don't want such
> > duplication.
> 
> Right, but the repository pom points to the eclipse-parent pom, and so it
> part of the inheritance chain.
> 
> PW

The goal is to have an eclipse-parent and an eclipse-test-parent pom, so that we can specify different compiler arguments globally.
Comment 65 Thanh Ha CLA 2013-08-23 10:39:21 EDT
(In reply to comment #64)
> (In reply to comment #63)
> > (In reply to comment #62)
> > > 
> > > No. The test pom still points to the repository pom and not the eclipse
> > > parent pom. That's where I'm currently with jdt.ui. But if I do that I will
> > > have to copy parts of the repo pom to the test pom and I don't want such
> > > duplication.
> > 
> > Right, but the repository pom points to the eclipse-parent pom, and so it
> > part of the inheritance chain.
> > 
> > PW
> 
> The goal is to have an eclipse-parent and an eclipse-test-parent pom, so
> that we can specify different compiler arguments globally.

If the goal is to only have compiler settings be different, maybe we can take advantage of "combine.self=override" maven property [1]. I haven't tried this myself but it sounds like we can make the regular "eclipse-parent pom" be the ultimate pom with all the common configurations and the "eclipse-test-parent" would inherit from the eclipse-parent but override the compiler settings so it doesn't inherit any of compiler related configuration.


[1] http://stackoverflow.com/questions/1771561/maven-is-it-possible-to-override-the-configuration-of-a-plugin-already-defined
Comment 66 Dani Megert CLA 2013-08-23 10:47:31 EDT
(In reply to comment #65)
> (In reply to comment #64)
> > (In reply to comment #63)
> > > (In reply to comment #62)
> > > > 
> > > > No. The test pom still points to the repository pom and not the eclipse
> > > > parent pom. That's where I'm currently with jdt.ui. But if I do that I will
> > > > have to copy parts of the repo pom to the test pom and I don't want such
> > > > duplication.
> > > 
> > > Right, but the repository pom points to the eclipse-parent pom, and so it
> > > part of the inheritance chain.
> > > 
> > > PW
> > 
> > The goal is to have an eclipse-parent and an eclipse-test-parent pom, so
> > that we can specify different compiler arguments globally.
> 
> If the goal is to only have compiler settings be different, maybe we can
> take advantage of "combine.self=override" maven property [1]. I haven't
> tried this myself but it sounds like we can make the regular "eclipse-parent
> pom" be the ultimate pom with all the common configurations and the
> "eclipse-test-parent" would inherit from the eclipse-parent but override the
> compiler settings so it doesn't inherit any of compiler related
> configuration.

But this wouldn't solve the issue we have at the repository level where the test pom needs to inherit from two sources: the repo pom and the eclipse-test-parent pom.
Comment 67 Thanh Ha CLA 2013-08-23 10:50:50 EDT
(In reply to comment #66)
> But this wouldn't solve the issue we have at the repository level where the
> test pom needs to inherit from two sources: the repo pom and the
> eclipse-test-parent pom.

Oh I see what you mean now. Let me think about this a bit more.
Comment 68 Dani Megert CLA 2013-08-23 10:54:05 EDT
(In reply to comment #67)
> (In reply to comment #66)
> > But this wouldn't solve the issue we have at the repository level where the
> > test pom needs to inherit from two sources: the repo pom and the
> > eclipse-test-parent pom.
> 
> Oh I see what you mean now. Let me think about this a bit more.


Here's another solution that might be easier:
- we define two variables in the parent pom:
  test-arguments and production-arguments
- in the repo pom and in the repo test pom we set the compile arguments using
  one of those variables
Comment 69 David Williams CLA 2013-08-25 23:10:18 EDT
I committed two variables to master, as the "starting point" for what these might look like. 

http://git.eclipse.org/c/platform/eclipse.platform.releng.aggregator.git/commit/?id=44cf9720d01a190d464e9d3de45b6d2c306bb570

I prefer the names "code-compilerArgs" and "test-compilerArgs" since a) their short and same size :) and b) I think "production" vs. "test" is confusing with the concept of "test builds" vs. "production builds". 

I realize what is currently used for code-compilerArgs

<code-compilerArgs>-warn:-deprecation,raw,unchecked</code-compilerArgs>

must be "tightened up", but wanted to do initial test run with it exactly the same, to be sure results were exactly the same (i.e. that the build still works). 

Assuming it does, what is desired Eclipse-wide defaults that you'd recommend? 
(for code- and test-compilerArgs). Thanks.
Comment 70 David Williams CLA 2013-08-26 00:46:41 EDT
(In reply to comment #69)
> ...
> 
> I realize what is currently used for code-compilerArgs
> 
> <code-compilerArgs>-warn:-deprecation,raw,unchecked</code-compilerArgs>
> 
> must be "tightened up", but wanted to do initial test run with it exactly
> the same, to be sure results were exactly the same (i.e. that the build
> still works). 
> 
> Assuming it does, what is desired Eclipse-wide defaults that you'd
> recommend? 
> (for code- and test-compilerArgs). Thanks.

My (local) test build did work fine and gave same warnings as current builds, so I think next steps are to define them to the values you'd like ... 

I'd guess, from a quick read of some of the early comments, we'd want 

<code-compilerArgs>-warn:-deprecation</code-compilerArgs>

and leave test- as I have it ... but, just realized, I may have pasted an extra '-' for "discouraged" ... not sure if two '-'s make a plus? 

<test-compilerArgs>-warn:-deprecation,raw,unchecked,-discouraged,forbidden,warningToken</test-compilerArgs>

And, I suggest you try out the ${test-compilerArgs} in jdt-ui and see if it works/builds with expected results ... once happy with that, we could announce to team. Or, similar.
Comment 71 Dani Megert CLA 2013-08-26 06:15:09 EDT
Is there a pattern/rule to choose '-' or '.' or camel case to separate words? Currently it looks pretty random to me.

I've changed them a bit, adjusted the values and the comment:
http://git.eclipse.org/c/platform/eclipse.platform.releng.aggregator.git/commit/?id=399b6975455d3c2559a0c1de552a6508f585b376


I guess the test bundle can't simply redefine the property that the eclipse-parent pom uses (would be cool though):

  <properties>
    <code.ignoredWarnings>${tests.ignoredWarnings}</code.ignoredWarnings>
  </properties>

i.e. we have to override the settings with:

  <build>
    <plugins>
      <plugin>
        <groupId>org.eclipse.tycho</groupId>
        <artifactId>tycho-compiler-plugin</artifactId>
        <version>${tycho.version}</version>
        <configuration>
          <compilerArgument>${tests.ignoredWarnings}</compilerArgument>
        </configuration>
      </plugin>
    </plugins>
  </build>

?
Comment 72 David Williams CLA 2013-08-26 08:11:17 EDT
(In reply to comment #71)
> Is there a pattern/rule to choose '-' or '.' or camel case to separate
> words? Currently it looks pretty random to me.

I googled for it this morning, and could not find any conventions, so, yes, I think random. I hope an expert can point us to a reference :) 

And I think your choices are fine. But, I think in theory, many "compiler args" could be in the <compilerArgument> element, so ... sorry to lose the fact that we are losing that part of name. I've always wanted to try a test case, if there was more time in a day, but I've never seen an example that combined 
"variables" with other text in an element, such as 

<compilerArgument>${tests.ignoredWarnings} -proc:none </compilerArgument>
 
But maybe would work? 


> 
> I guess the test bundle can't simply redefine the property that the
> eclipse-parent pom uses (would be cool though):
> 
>   <properties>
>     <code.ignoredWarnings>${tests.ignoredWarnings}</code.ignoredWarnings>
>   </properties>
> 

Not sure. It actually seems like it should work for simple cases? (But, I still recommend separate "test parent" for each repo ... just seems long term there might be more things done differently for tests.)
Comment 73 Dani Megert CLA 2013-08-26 08:23:40 EDT
(In reply to comment #72)
> (In reply to comment #71)
> > Is there a pattern/rule to choose '-' or '.' or camel case to separate
> > words? Currently it looks pretty random to me.
> 
> I googled for it this morning, and could not find any conventions, so, yes,
> I think random. I hope an expert can point us to a reference :) 
> 
> And I think your choices are fine. But, I think in theory, many "compiler
> args" could be in the <compilerArgument> element, so ... sorry to lose the
> fact that we are losing that part of name. 

It was intentional. For more complicated overrides people should use the non-deprecated <compilerArgs>. And, with that property name the pom in the leaves can be understood much easier.


> I've always wanted to try a test
> case, if there was more time in a day, but I've never seen an example that
> combined 
> "variables" with other text in an element, such as 
> 
> <compilerArgument>${tests.ignoredWarnings} -proc:none </compilerArgument>
>  
> But maybe would work? 

Not sure. We could run a test run along with trying the properties based approach? I can make the changes if you have time for a dry run (sorry still not setup my local build).


> (But, I
> still recommend separate "test parent" for each repo ... just seems long
> term there might be more things done differently for tests.)

Agreed.
Comment 74 David Williams CLA 2013-08-26 08:34:35 EDT
 
> > But maybe would work? 
> 
> Not sure. We could run a test run along with trying the properties based
> approach? I can make the changes if you have time for a dry run (sorry still
> not setup my local build).
> 

Plenty of time -- in Eastern time zone :). 
I'm currently doing a test build "as is". That should be done by 9:30 or 10:00 and should just as well let that finish, but could then start another, if you'd like ... done by noon or 1 pm (Eastern) ... as long as someone around to "revert" if didn't work.
Comment 75 David Williams CLA 2013-08-26 08:37:47 EDT
(In reply to comment #74)
>  
> > > But maybe would work? 
> > 
> > Not sure. We could run a test run along with trying the properties based
> > approach? I can make the changes if you have time for a dry run (sorry still
> > not setup my local build).
> > 
> 
> Plenty of time -- in Eastern time zone :). 
> I'm currently doing a test build "as is". That should be done by 9:30 or
> 10:00 and should just as well let that finish, but could then start another,
> if you'd like ... done by noon or 1 pm (Eastern) ... as long as someone
> around to "revert" if didn't work.

Just remembered one complication ... if we are making "pom only" changes, at some point do we have to "touch" each test bundle ... or is produced code identical and only "compiler output" different?
Comment 76 Dani Megert CLA 2013-08-26 08:46:41 EDT
(In reply to comment #74)
> Plenty of time -- in Eastern time zone :). 
> I'm currently doing a test build "as is". That should be done by 9:30 or
> 10:00 and should just as well let that finish, but could then start another,
> if you'd like ... done by noon or 1 pm (Eastern) ... as long as someone
> around to "revert" if didn't work.

I guess it would fail pretty quickly. I suggest we only tweak one aspect at a time. Preferably, I'd like to try to only set the property in the leave pom instead of using the current more complex syntax. Later we can try to combine the variables. Let me know when the ongoing build is done and I can make that change.


(In reply to comment #75)
> (In reply to comment #74)
> >  
> > > > But maybe would work? 
> > > 
> > > Not sure. We could run a test run along with trying the properties based
> > > approach? I can make the changes if you have time for a dry run (sorry still
> > > not setup my local build).
> > > 
> > 
> > Plenty of time -- in Eastern time zone :). 
> > I'm currently doing a test build "as is". That should be done by 9:30 or
> > 10:00 and should just as well let that finish, but could then start another,
> > if you'd like ... done by noon or 1 pm (Eastern) ... as long as someone
> > around to "revert" if didn't work.
> 
> Just remembered one complication ... if we are making "pom only" changes, at
> some point do we have to "touch" each test bundle ... or is produced code
> identical and only "compiler output" different?

Will we see the different/new compiler results? If so, it's fine. The produced code should be the same, plus, we only change(d) the settings for test bundles.
Comment 77 David Williams CLA 2013-08-26 10:10:22 EDT
(In reply to comment #76)
> (In reply to comment #74)
> > Plenty of time -- in Eastern time zone :). 
> > I'm currently doing a test build "as is". That should be done by 9:30 or
> > 10:00 and should just as well let that finish, but could then start another,
> > if you'd like ... done by noon or 1 pm (Eastern) ... as long as someone
> > around to "revert" if didn't work.
> 
> I guess it would fail pretty quickly. I suggest we only tweak one aspect at
> a time. Preferably, I'd like to try to only set the property in the leave
> pom instead of using the current more complex syntax. Later we can try to
> combine the variables. Let me know when the ongoing build is done and I can
> make that change.
> 

First test build completed ... looks normal.  

http://build.eclipse.org/eclipse/builds/4I/siteDir/eclipse/downloads/drops4/I20130826-0739/
Comment 78 David Williams CLA 2013-08-26 10:27:10 EDT
(In reply to comment #77)
> (In reply to comment #76)
> > (In reply to comment #74)

> 
> First test build completed ... looks normal.  
> 
> http://build.eclipse.org/eclipse/builds/4I/siteDir/eclipse/downloads/drops4/
> I20130826-0739/

Dani has made change to try simply, to confirm it's "inherited" as expected. 

  <properties>
    <code.ignoredWarnings>${tests.ignoredWarnings}</code.ignoredWarnings>
  </properties>

Test build started. Results will be at URL below in a few hours (since I list now, since I'll be away a few hours). 

http://build.eclipse.org/eclipse/builds/4I/siteDir/eclipse/downloads/drops4/I20130826-1018/
Comment 79 David Williams CLA 2013-08-26 12:50:39 EDT
(In reply to comment #78)
> (In reply to comment #77)
> > (In reply to comment #76)
> > > (In reply to comment #74)
> 
> > 
> > First test build completed ... looks normal.  
> > 
> > http://build.eclipse.org/eclipse/builds/4I/siteDir/eclipse/downloads/drops4/
> > I20130826-0739/
> 
> Dani has made change to try simply, to confirm it's "inherited" as expected. 
> 
>   <properties>
>     <code.ignoredWarnings>${tests.ignoredWarnings}</code.ignoredWarnings>
>   </properties>
> 
> Test build started. Results will be at URL below in a few hours (since I
> list now, since I'll be away a few hours). 
> 
> http://build.eclipse.org/eclipse/builds/4I/siteDir/eclipse/downloads/drops4/
> I20130826-1018/

I seems to me that the "property only" change (also) fixed the issue ... 

So, what's next? 

Dani, will you (please) prepare "instructions" for others?
Comment 80 Dani Megert CLA 2013-08-26 13:42:04 EDT
(In reply to comment #79)
> I seems to me that the "property only" change (also) fixed the issue ... 

Indeed! I verified the properties in the XML file.

 
> So, what's next? 
> 
> Dani, will you (please) prepare "instructions" for others?

I've converted JDT UI and Platform UI for the next N-build. If that works fine, I'll convert some others and send a note for the remaining repos.


I also changed the eclipse-parent pom to use code.ignoredWarnings to set the value for tests.ignoredWarnings. David is currently running a test build for that.
Comment 81 David Williams CLA 2013-08-26 13:53:24 EDT
> 
> I also changed the eclipse-parent pom to use code.ignoredWarnings to set the
> value for tests.ignoredWarnings. David is currently running a test build for
> that.

And, that build failed relatively quickly, during the "pom-updater" phase ... something about " recursive expression cycle".

http://build.eclipse.org/eclipse/builds/4I/siteDir/eclipse/downloads/drops4/I20130826-1340/buildFailed-pom-version-updater

Did that test what you wanted to see?
Comment 82 Dani Megert CLA 2013-08-26 14:04:12 EDT
(In reply to comment #81)
> > 
> > I also changed the eclipse-parent pom to use code.ignoredWarnings to set the
> > value for tests.ignoredWarnings. David is currently running a test build for
> > that.
> 
> And, that build failed relatively quickly, during the "pom-updater" phase
> ... something about " recursive expression cycle".
> 
> http://build.eclipse.org/eclipse/builds/4I/siteDir/eclipse/downloads/drops4/
> I20130826-1340/buildFailed-pom-version-updater
> 
> Did that test what you wanted to see?

I guess so. It looks like we cannot use a property inside the same properties section. I've reverted that change. Let's try again.
Comment 83 Dani Megert CLA 2013-08-26 15:57:11 EDT
(In reply to comment #80)
> > So, what's next? 
> I've converted JDT UI and Platform UI for the next N-build. If that works
> fine, I'll convert some others and send a note for the remaining repos.

Converted Platform Text.
Comment 84 Dani Megert CLA 2013-08-27 10:45:07 EDT
I've fixed it for the following components:

JDT Debug
PDE Build
PDE UI

Ant
Debug
Releng
Resources
Runtime
Team
UA
UI

and filed bugs for the remaining ones (see dependent bugs).
Comment 85 Dani Megert CLA 2013-09-17 03:12:00 EDT
All dependent bugs got fixed. Closing.