search

maven-nar / nar-maven-plugin

Native ARchive plugin for Maven
https://maven-nar.github.io/
Apache License 2.0
232 stars 160 forks source link

Feature system includes #327

Closed bchiodo closed 5 years ago

bchiodo commented 6 years ago

This PR allows you to specify whether dependent projects should treat included header files as system headers (and thus ignoring warnings).

At my company we package most of our projects' system dependencies as nar artifacts. Things like boost, googletest, fftw etc. However, the problem with this approach is that the headers from these projects aren't treated as system headers by g++ because nar specifies them with the -I flag (and not -isystem).

Consequently, our build logs are polluted with a bunch of warnings from system headers. This PR allows you to specify via the includesType option whether the artifact headers should be treated as a system headers by dependent projects.

There are a couple of problems with this PR however so I am not sure it should be accepted as-is.

  1. The integration test is GCC-only as it enables -Werror in order to fail the failure case. We are a linux only shop so I don't have the resources (or ability...) to write a visual studio test case.
  2. The way this works is that it writes out the includesType=system|local line to the generated nar.properties file. The problem with this approach is that the nar.properties file is not included in the noarch attachment only AOL-specific ones. See file 'NarPreparePackageMojo.java'.

What that final bullet implies is that the option will have no effect if the artifact is a header-only library and only has a noarch artifact. That is why the test cases actually build a library when they otherwise need not. I am not sure of an easy solution to this problem without changing some significant parts of the plugin so I am accepting of the limitation as is. Feedback, of course, is welcome.

bchiodo commented 5 years ago

Any chance this could get merged? We have been using it here on many projects without issue for the last year.

ctrueden commented 5 years ago

@bchiodo Thanks for the ping, and sorry this sat unanswered for so long. I'll try to work on this in the next couple of days. In the meantime: would you have time to rebase and force push a patch set without conflicts?

bchiodo commented 5 years ago

No worries on not getting to it for a while. I just pinged now because I may need to make some more changes to nar in the future and didn't want too many outstanding branches to keep track of.

I did the fetch/rebase/force-push dance and that should resolve the conflicts. I also updated the tests for the PR because they were broken on my current gcc version (7.5).

ctrueden commented 5 years ago

Thanks, @bchiodo!