Windows msvc #181

GregDomjan · 2015-06-17T20:02:37Z

Some cleanup to recent addition of msvc config
Changes to support Windows specific compilations such as resource only dll with no manifest
Remove from aol config properties that where defined by compiler
Fix multi thread/core bug
Enhance to work in project with multiple dependency types.
Refactor artifact filtering and add option to config further filtering

Executing via cmd causes issues with long command lines and adds security issues. Setting environment vars only set last CompilerDef when multiple might be present, go through CompilerDef list and set all Setting newEnvironment now flows through from Compile/Linker Def to implementation.

Without the step of configuring cmd with new path/env the tools need initial information for their full path, adding toolpath setting. With clearing of the environment (setting Path env) need to also add SystemRoot and TMP environment vars (perhaps clear environment should be optional to turn off, and default clears when setting any var) Adjust Path configuration to better match the various native/cross settings as per vcvars.bat and including minimal basic reference to windows system Add atlmfc to paths (perhaps should be optional)

Linker manifest generation now optional. Manifest may be provided as source. MT manifest embedding now optional. In some cases it is preferred to provide the manifest Side by Side. For some modules like resource only don't need manifest at all.

Windows 8+ sdk includes use of the symbol Windows such as in shldisp.h Also _WIN32 and _WIN64 are defined by cl compiler and don't need to be defined again. Some earlier versions cl (<=6) defined Windows _WINDOWS which perhaps should be applied based on the linker version Uncertain on appropriate to remove from gcc/gpp sections

Run Precompile steps and wait for completion before moving on to others. Identify Message and IDL compile as 'precompile' steps.

This returns some behaviour that was present before re-factoring for eclipse support. In some cases the dependent nar libraries won't have a suitable default inclusion lib and windows #pragma message(lib,) is used to be more specific. boost and wxWidgets are 2 examples

Use configured linkage if provided. Rather than fixed default of Shared linkage, if a single library can be selected use library type.

NAR may not be the only dependency type, should not expect all dependencies to be of archive type accessible as jar, and should not go rummaging through other types looking for NAR data even if it is a jar and can be opened.

When not working with NAR packaging, assume all available Library types should be download/unpacked.

ctrueden · 2015-06-17T20:15:25Z

Cool, thanks for all the work. I have some concerns about breaking API changes. But in the interest of time, if you are confident these are all good changes, then I'm fine going to 4.0.0 with the next release.

ctrueden · 2015-06-17T20:16:17Z

@pauldaustin Since a lot of this concerns your recent work, would you mind contributing to this PR with a code review? Personally I don't really feel comfortable judging the technical details.

GregDomjan · 2015-06-17T20:24:25Z

I get where your coming from for semantic versioning and API, though I'm not feeling we have an API as such beyond the configuration. I'm not against this becoming 4.0.0.

ctrueden · 2015-06-17T20:27:31Z

Going to 4.0.0 seems easiest to me. I don't care too much if we end up at 20.0.0, since as you point out there are not really any public users of NAR as a library. If that changes, we can start being more conservative about things like public method signature changes.

As for reviewing the functional meat of these changes: is there anyone besides @pauldaustin that we should invite to have a look?

GregDomjan · 2015-06-17T21:03:06Z

Perhaps @dugilos could help review on changes for artifact selection/filtering.

In the groups Thomas Haitzer recently asked about msvc and Henrik Horneber was asking about resource compilation.

ctrueden · 2015-06-17T21:09:03Z

Might be worth posting to the group about this PR then, to invite other potential reviewers.

buildhive · 2015-06-17T21:23:04Z

NAR plugin for Maven » nar-maven-plugin #349 SUCCESS
This pull request looks good
(what's this?)

remove dead code.

pauldaustin · 2015-06-18T03:53:56Z

Look at the diffs for the following files.

src/main/java/com/github/maven_nar/cpptasks/compiler/CommandLineCompiler.java
src/main/java/com/github/maven_nar/cpptasks/compiler/CommandLineLinker.java
You need code like the following otherwise the PATH doesn’t get passed to the command correctly. Ideally we should have a CommandLine executor wrapper that on windows would launch the command with cmd /c before the actual command.

if (NarUtil.isWindows()) {

```
 allArgsCount += 2;  
```
}
if (NarUtil.isWindows()) {
```
 allArgs[index++] = "cmd";  
```
```
 allArgs[index++] = "/c";  
```
}

Make sure the changes work with different windows SDK and MSVC versions. different windows SDK have different paths and include directories

On 17 Jun 2015, at 13:27, Curtis Rueden notifications@github.com wrote:

Going to 4.0.0 seems easiest to me. I don't care too much if we end up at 20.0.0, since as you point out there are not really any public users of NAR as a library. If that changes, we can start being more conservative about things like public method signature changes.

As for reviewing the functional meat of these changes: is there anyone besides @pauldaustin https://github.com/pauldaustin that we should invite to have a look?

—
Reply to this email directly or view it on GitHub #181 (comment).

GregDomjan · 2015-06-18T04:24:53Z

Thanks @pauldaustin
I have been testing mainly with VS 2013, but will check again with VS 2008, 2010, 2012. I don't seem to have 2009 installed anywhere... I've also been testing with SDK 7.0 through 8.1

Including cmd needs to be unnecessary as there are too many issues with starting to use it, I would say it's far from ideal to introduce it.
Using cmd.exe introduces limits that need not be present when executing more directly, it also introduces possibility of injections and mangling/munging.
I'd like to look at using org.codehaus.plexus:plexus-utils CommandLine class as they have already dealt with many issues. This might also be way forward with #179

The PATH is now resolved correctly, and settings from the user are not accidentally injected causing or hiding problems. Without the addition of the SystemRoot and TMP msvc would fail even with a good path. Here is where using cmd was giving an injection already.
Also required providing full path to the initial executable instead of relying on the cmd environment.

thohai · 2015-06-18T06:38:40Z

Hi!
Unfortunately I am just a "user" of the msvc-code and did not even have a glance on it sofar.
So please take this into account when reading the next view lines:
I looked through the changes and could not find anything I would object to. I think that getting rid of the /cmd dependency is a good thing, especially if it means that the VC command prompt is no longer needed for executing the nar plugin. (This one I found rather unintuitive.)

On a side note: if I saw this correctly, this code is not ready for Windows 10 yet.
Hope this helps.

Thomas

pauldaustin · 2015-06-18T14:30:11Z

@GregDomjan

I assume by the following you mean that you set the tool path so that the command line has the full path to the command executed.

Also required providing full path to the initial executable instead of relying on the cmd environment.

As long as the following still works I'm fine with the change

No need to launch the build from a Visual Studio command prompt with support for the auto-detection of the Visual Studio location.
Support for both x86 and 64 bit systems
Support for cross compiling (so x84 from a 64 bit compiler)
Support for SDK versions 7.1A, 8.0, 8.1
Support for at least VS 2008, 2010, 2013

Prefer mt.exe from latest windows sdk if not available in sdk (pre7) and using visual studio 2005 use it otherwise leave it to be found on the path

8.0 was being disallowed due to \d\d.\d pattern later substring failed due to 2 digit expectation. refactored removing substring to using regex match groups Additional debug reporting of identified system environment

GregDomjan · 2015-06-19T00:52:50Z

Previously I missed an issue with mt.exe not using toolpath when changed my build to a visual studio cmd prompt.

There are some differences between Windows SDK 6.0A/7.0A/7.1A (installed with visual studio) and 6.0/7.0/7.1 (stand alone windows platform sdk) but I'm not sure if it's acceptable to configure say 7.1A and 'automatically upgrade' to 7.1 - probably not good to 'downgrade' from 7.1 to 7.1A. To me they are generally interchangeable so it feels like it should be more flexible instead of pedantic.
Since 8 visual studio seems to just install the full platform kit and so variation is no longer present.

GregDomjan · 2015-06-19T00:56:31Z

@thohai I'm not aware of any issue this may face with running on Windows 10, perhaps you could elaborate?
Hmm, is it that they have changed the format of the kits folder name to not have a .minor - just 10
Now I have it installed I see an extra subfolder too.
C:\Program Files (x86)\Windows Kits\10\Include\10.0.10069.0\um

buildhive · 2015-06-19T02:01:25Z

NAR plugin for Maven » nar-maven-plugin #353 SUCCESS
This pull request looks good
(what's this?)

On OS X, through JDK 6 JNI libraries used .jnilib, but have since switched to .dylib. Change the generated NarSystem.loadLibrary that works with native-lib-loader to work with either extension.

Check for .jnilib and .dylib on OS X

A fix to "NAR: Compile failed: Output filename conflict"

ctrueden · 2015-08-11T21:28:43Z

@GregDomjan @pauldaustin @dugilos So the consensus so far is that all of these changes are good as-is?

If so: @GregDomjan: if you have time to fix the merge conflicts, we can go ahead and merge whenever you're ready.

Do not check non-jar or non-nar artefacts for NarInfo

dugilos · 2015-08-12T18:01:05Z

Hello, the IT test 28 that I made to test the changes I made is still OK, so for me there is no reserve.

ctrueden · 2015-08-12T18:15:02Z

@GregDomjan The merge conflicts were super easy to resolve, so I just went ahead and did it. 😄

Windows msvc

ctrueden · 2015-08-12T18:16:42Z

Continuing the shotgun approach of merging all the PRs, and then fixing any later brokenness if/when it arises. Especially since this one has a lot of nice cleanup. Thanks @GregDomjan!

buildhive · 2015-08-12T19:34:56Z

NAR plugin for Maven » nar-maven-plugin #358 SUCCESS
This pull request looks good
(what's this?)

GregDomjan · 2015-08-12T20:12:22Z

Thanks, was just getting around to doing the same.

Oneplus and others added 18 commits June 12, 2015 20:07

fix output conflict

fbb5afd

MSVC requires architecture/os before init

af4ee25

Make dependency include path optional, warn when missing.

3ff2c52

Manifest only required for dll/exe - not static

aa88200

Dependencies of Precompile failing on multi core machine

7167a47

Run Precompile steps and wait for completion before moving on to others. Identify Message and IDL compile as 'precompile' steps.

Not all test builds need be executable

cd0f217

Automate selection of default test linkage

e1f97e3

Use configured linkage if provided. Rather than fixed default of Shared linkage, if a single library can be selected use library type.

Play nice with others

4438682

NAR may not be the only dependency type, should not expect all dependencies to be of archive type accessible as jar, and should not go rummaging through other types looking for NAR data even if it is a jar and can be opened.

Remove unecessary mojo passing to msvc

c22330e

Allow executables to be unpacked as a dependency.

3db08cd

When not working with NAR packaging, assume all available Library types should be download/unpacked.

Use maven-common-artifact-filters for manipulating artifact lists

a0c61af

Allow filtering of dependencies by ArtifactIds or GroupIds

e8aa2e6

Cleanup dead code from NarManager

8906f94

String[] -> ArrayList<String> to eliminate possible skew

430f42d

remove dead code.

GregDomjan added 3 commits June 18, 2015 18:33

Don't override configured linker toolPath

893a8c3

Fix issues with path to mt.exe

8c93bb1

Prefer mt.exe from latest windows sdk if not available in sdk (pre7) and using visual studio 2005 use it otherwise leave it to be found on the path

Fix issues with MSVC/VS single digit major version

c39852b

8.0 was being disallowed due to \d\d.\d pattern later substring failed due to 2 digit expectation. refactored removing substring to using regex match groups Additional debug reporting of identified system environment

Check for .jnilib and .dylib on OS X

b758556

On OS X, through JDK 6 JNI libraries used .jnilib, but have since switched to .dylib. Change the generated NarSystem.loadLibrary that works with native-lib-loader to work with either extension.

ctrueden mentioned this pull request Aug 4, 2015

Do not check non-jar or non-nar artefacts for NarInfo #185

Merged

ctrueden added 2 commits August 11, 2015 14:43

Merge pull request #184 from diyessi/osx-jnidylib

c0354be

Check for .jnilib and .dylib on OS X

Merge pull request #179 from Oneplus/fix-output-conflict

a6f2801

A fix to "NAR: Compile failed: Output filename conflict"

Merge pull request #185 from markBowman/master

283fbb0

Do not check non-jar or non-nar artefacts for NarInfo

Merge branch 'master' into 'windows-msvc'

c4a050d

ctrueden added a commit that referenced this pull request Aug 12, 2015

Merge pull request #181 from maven-nar/windows-msvc

66616c4

Windows msvc

ctrueden merged commit 66616c4 into master Aug 12, 2015

ctrueden deleted the windows-msvc branch August 12, 2015 18:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Windows msvc #181

Windows msvc #181

GregDomjan commented Jun 17, 2015

ctrueden commented Jun 17, 2015

ctrueden commented Jun 17, 2015

GregDomjan commented Jun 17, 2015

ctrueden commented Jun 17, 2015

GregDomjan commented Jun 17, 2015

ctrueden commented Jun 17, 2015

buildhive commented Jun 17, 2015

pauldaustin commented Jun 18, 2015

GregDomjan commented Jun 18, 2015

thohai commented Jun 18, 2015

pauldaustin commented Jun 18, 2015

GregDomjan commented Jun 19, 2015

GregDomjan commented Jun 19, 2015

buildhive commented Jun 19, 2015

ctrueden commented Aug 11, 2015

dugilos commented Aug 12, 2015

ctrueden commented Aug 12, 2015

ctrueden commented Aug 12, 2015

buildhive commented Aug 12, 2015

GregDomjan commented Aug 12, 2015

Windows msvc #181

Windows msvc #181

Conversation

GregDomjan commented Jun 17, 2015

ctrueden commented Jun 17, 2015

ctrueden commented Jun 17, 2015

GregDomjan commented Jun 17, 2015

ctrueden commented Jun 17, 2015

GregDomjan commented Jun 17, 2015

ctrueden commented Jun 17, 2015

buildhive commented Jun 17, 2015

pauldaustin commented Jun 18, 2015

GregDomjan commented Jun 18, 2015

thohai commented Jun 18, 2015

pauldaustin commented Jun 18, 2015

GregDomjan commented Jun 19, 2015

GregDomjan commented Jun 19, 2015

buildhive commented Jun 19, 2015

ctrueden commented Aug 11, 2015

dugilos commented Aug 12, 2015

ctrueden commented Aug 12, 2015

ctrueden commented Aug 12, 2015

buildhive commented Aug 12, 2015

GregDomjan commented Aug 12, 2015