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

Mingw link issue #3270

Merged
merged 7 commits into from Feb 15, 2019

Conversation

Projects
None yet
4 participants
@dmoody256
Copy link
Contributor

commented Jan 14, 2019

The test/Win32/mingw.py test on my local system and on Appveyor 2017 image. We run the 2015 image which the test gets skipped in.

These changes allow the test to pass.

Changes in this PR:

  • Update link.py to validate targets as Nodes
  • Update mingw.py to remove /nologo flag added by msvc tool
  • Update appveyor to use 32 bit mingw until this is done: #3291

Contributor Checklist:

  • I have updated master/src/CHANGES.txt directory (and read the README.txt in that directory)

dmoody256 added some commits Jan 14, 2019

fixed mingw emitter to convert str to node before accessing node memb…
…er, and updated mingw to remove MSVC like nologo flag
@coveralls

This comment has been minimized.

Copy link

commented Jan 14, 2019

Coverage Status

Coverage decreased (-0.0009%) to 79.998% when pulling 4518d14 on dmoody256:mingw_link_issue into 0f782d0 on SCons:master.

3 similar comments
@coveralls

This comment has been minimized.

Copy link

commented Jan 14, 2019

Coverage Status

Coverage decreased (-0.0009%) to 79.998% when pulling 4518d14 on dmoody256:mingw_link_issue into 0f782d0 on SCons:master.

@coveralls

This comment has been minimized.

Copy link

commented Jan 14, 2019

Coverage Status

Coverage decreased (-0.0009%) to 79.998% when pulling 4518d14 on dmoody256:mingw_link_issue into 0f782d0 on SCons:master.

@coveralls

This comment has been minimized.

Copy link

commented Jan 14, 2019

Coverage Status

Coverage decreased (-0.0009%) to 79.998% when pulling 4518d14 on dmoody256:mingw_link_issue into 0f782d0 on SCons:master.

@coveralls

This comment has been minimized.

Copy link

commented Jan 14, 2019

Coverage Status

Coverage decreased (-0.02%) to 79.983% when pulling c2dc898 on dmoody256:mingw_link_issue into 0f782d0 on SCons:master.

@bdbaddog

This comment has been minimized.

Copy link
Contributor

commented Jan 14, 2019

Where is /NOLOGO getting set in the first place?
Wouldn't it be better to not set it?

Also a unit test can check this is working.

@dmoody256

This comment has been minimized.

Copy link
Contributor Author

commented Jan 14, 2019

Where is /NOLOGO getting set in the first place?
Wouldn't it be better to not set it?

I think both maybe, I suppose it matters the order the tools are loaded. It is set from the msvc.py tool, and for the test/Win32/mingw.py test, it creates a Environment(tools=['default', 'mingw']) which I think means the msvc tool runs first, adding the flag, then the mingw tool runs, removing the flag.

Another case could be Environment(tools=['mingw', 'msvc']) which would not remove the flag. I guess the msvc tool should check what tools are loaded and make sure not to add that flag if the mingw tool was loaded? The test/Win32/mingw.py should probably be updated to test this case also.

Also a unit test can check this is working.

Ok, I'll add a unit test so we can see it passing in the Appveyor 2015 image for this PR.

@dmoody256 dmoody256 changed the title Mingw link issue [WIP] Mingw link issue Jan 14, 2019

@bdbaddog

This comment has been minimized.

Copy link
Contributor

commented Jan 15, 2019

Maybe add a unit test, but file a bug which says mingw and msvc stomp on each others flags?
With a short repeater?

@dmoody256 dmoody256 referenced this pull request Jan 20, 2019

Merged

Switch to codecov #3275

@@ -167,7 +167,8 @@ def generate(env):

env['SHOBJSUFFIX'] = '.o'
env['STATIC_AND_SHARED_OBJECTS_ARE_THE_SAME'] = 1

if 'CCFLAGS' in env:
env['CCFLAGS'] = SCons.Util.CLVar(str(env['CCFLAGS']).replace('/nologo', ''))

This comment has been minimized.

Copy link
@bdbaddog

bdbaddog Jan 20, 2019

Contributor

Won't this fail badly if CCFLAGS is a list?

This comment has been minimized.

Copy link
@dmoody256

dmoody256 Jan 21, 2019

Author Contributor

yeah, I was assuming it was CLVar, but its user settable so really should check if its CLVar and if not make it one first before doing the replace.

@@ -150,6 +150,7 @@ def generate(env):

#... but a few things differ:
env['CC'] = 'gcc'
env['CCFLAGS'] = SCons.Util.CLVar('')

This comment has been minimized.

Copy link
@bdbaddog

bdbaddog Jan 27, 2019

Contributor

with something like

env=Environment(tools=['mslink','mingw','g++'],CCFLAGS='-myflag')

with the new code CCFLAGS will be ignored?

This comment has been minimized.

Copy link
@dmoody256

dmoody256 Jan 28, 2019

Author Contributor

I was looking at the MSVC tool and it seemed to do the same thing, so I
thought that was ok to remove flags during generate:

env['CCFLAGS'] = SCons.Util.CLVar('/nologo')
env['CFLAGS'] = SCons.Util.CLVar('')

@bdbaddog

This comment has been minimized.

Copy link
Contributor

commented Jan 27, 2019

Seems like the best fix here is probably to fix the tests, and then for mingw to remove any /options ? (or to issue warnings about / options)

@dmoody256

This comment has been minimized.

Copy link
Contributor Author

commented Jan 28, 2019

Seems like the best fix here is probably to fix the tests,

Not sure what you mean by fix the tests, they are explicitly testing that msvc and mingw work together:

scons/test/Win32/mingw.py

Lines 165 to 166 in e44c771

# Test with specifying the default tool to make sure msvc setting doen't ruin it
# for mingw:

I can add unit test such that fakes out the need for mingw to actually be present, and check that certain things were setup in the environment, like flags that don't have / in them.

and then for mingw to remove any /options ? (or to issue warnings about / options)

It would have to remove it, because the msvc tool is adding it, and seems like the test indicates those tools should be able to be in the same environment.

@@ -81,6 +81,8 @@ def _lib_emitter(target, source, env, **kw):
if Verbose:
print("_lib_emitter: target[0]={!r}".format(target[0].get_path()))
for tgt in target:
if SCons.Util.is_String(tgt):

This comment has been minimized.

Copy link
@bdbaddog

bdbaddog Jan 28, 2019

Contributor

Curious why this started failing now?

This comment has been minimized.

Copy link
@dmoody256

dmoody256 Jan 29, 2019

Author Contributor

I don't know either, I don't know the last time we had the test/Win32/mingw.py test passing to see what changed since then.

This comment has been minimized.

Copy link
@mwichmann

mwichmann Jan 29, 2019

Collaborator

A bit of a weakness of our tests which just skip a test if the tool/setup is not there is if no CI environment is set up for a particular config, it just doesn't get tested. We have potential exposure on mingw, D, docbook, TeX, SWIG, packaging. Some of these have gotten added in recently, others fail to be added (e.g. the WiX toolset for msi packaging: apparently that code has rotted, or WiX has changed out from underneath it).

This comment has been minimized.

Copy link
@bdbaddog

bdbaddog Jan 29, 2019

Contributor

Indeed. Some tests will create fake tools to run with always and/or if the tool doesn't exist in the environment. However in some cases that's insufficient to test if the scons code works. Ideally we'd get a list of all tools missing and/or causing tests to be skipped at the end of a test run.. but that could be considerable work to make happen.

Any thoughts on how to better address?

This comment has been minimized.

Copy link
@dmoody256

dmoody256 Jan 29, 2019

Author Contributor

Both Appveyor and Travis have good REST APIs for getting the logs. I could fashion a python script to get the logs of the latest master CI runs and parse the No Results at the end. That would make collecting the current list easier.

We could take that list and update a wiki page using the wiki git repository like in this example: https://stackoverflow.com/a/14673994/1644736

This comment has been minimized.

Copy link
@dmoody256

dmoody256 Feb 6, 2019

Author Contributor

Yeah, it will get an exception that 'str' has no member 'attributes' just below. I'm not sure if there is somewhere further up the chain that the target should have been converted.

This comment has been minimized.

Copy link
@dmoody256

dmoody256 Feb 6, 2019

Author Contributor

Still should be some kind of check here even if the real fix is somewhere else. Maybe a try/except? Add a warning message? I think assuming its a file if its a string is a pretty good default case.

This comment has been minimized.

Copy link
@bdbaddog

bdbaddog Feb 6, 2019

Contributor

Yes. Can you change to try/except and put a comment in the except that we'd expect the target to be a Node at this point. And a # TODO to track down the why at some point?
I don't want to lose this discovery without some reasonable way of remembering to address it at some point if possible.

This comment has been minimized.

Copy link
@dmoody256

dmoody256 Feb 8, 2019

Author Contributor

I chased this down and found that mingw is getting an emitter list of [mslink.py winLibEmitter, link.py shlib_emitter, mingw.py shlib_emitter]. mslink's winLibEmitter never converts its new targets to File's and just leaves them as strings:

extratargets.append(
env.ReplaceIxes(dll,
'%sPREFIX' % paramtp, '%sSUFFIX' % paramtp,
"LIBPREFIX", "LIBSUFFIX"))
# and .exp file is created if there are exports from a DLL
extratargets.append(
env.ReplaceIxes(dll,
'%sPREFIX' % paramtp, '%sSUFFIX' % paramtp,
"WINDOWSEXPPREFIX", "WINDOWSEXPSUFFIX"))
return (target+extratargets, source+extrasources)

The other tools with emitters take care to convert targets to File objects:
target_strings = env.ReplaceIxes(orig_target[0],
'%sPREFIX' % vp, '%sSUFFIX' % vp,
'IMPLIBPREFIX', 'IMPLIBSUFFIX')
if Verbose:
print("_lib_emitter: target_strings=%r" % target_strings)
implib_target = env.fs.File(target_strings)

So the fix can be applied by converting the the targets in mslink.py's winLibEmitter to File's. Probably still want to have a check in link.py to convert str's as well.

This comment has been minimized.

Copy link
@dmoody256

dmoody256 Feb 8, 2019

Author Contributor

I left the if check instead of a try except because the current code looks cleaner than:

 for tgt in target:
        try:
            tgt.attributes.shared = 1
        except AttributeError:
            tgt = env.File(tgt)
            tgt.attributes.shared = 1

I am not sure about the speed up. Let me know your opinion.

@dmoody256 dmoody256 force-pushed the dmoody256:mingw_link_issue branch from c2dc898 to dd340d5 Jan 30, 2019

@dmoody256

This comment has been minimized.

Copy link
Contributor Author

commented Feb 3, 2019

So now with the AppendENVPath (#3281) fixed, mingw.py test is having an issue because its using the 32 bit MinGW which is first in the tools default paths. Previously it would get pushed to the back and the 64 bit mingw would be next in line. Appveyor sets an environment path for 64 bit mingw, which then fails to run the 32 bit executable. It seems we would need to update the appveyor path to point to 32 bit MinGW.

Which begs the question, what if someone had multiple versions of a tool installed, how would they force scons to make sure the desired one is chosen? Seems like they would need to call PrependENVPath with which ever one they wanted after the tool was loaded.

@dmoody256 dmoody256 force-pushed the dmoody256:mingw_link_issue branch from 6d2d97f to b3f363b Feb 3, 2019

@mwichmann

This comment has been minimized.

Copy link
Collaborator

commented Feb 3, 2019

Which begs the question, what if someone had multiple versions of a tool installed, how would they force scons to make sure the desired one is chosen? Seems like they would need to call PrependENVPath with which ever one they wanted after the tool was loaded.

I've wondered that... having several java versions installed isn't that uncommon (although maybe multiple jdks is less common). We include JAVA_HOME as an argument to the build to help with that.

@bdbaddog

This comment has been minimized.

Copy link
Contributor

commented Feb 3, 2019

@dmoody256

This comment has been minimized.

Copy link
Contributor Author

commented Feb 6, 2019

This is what we added TARGET_ARCH for (well 64 vs 32 bits), but currently it's only used for MSVC. Is there an easy way to determine if the installled mingw is 32 vs 64 bits and could be used to select the correct installed version? Also MINGW_VERSION maybe to say we only want a specific version (also copying from MSVC)?

I am going to write that as a separate issue, and for this PR focus on getting mingw working with the first version it finds. Created #3291 for that functionality.

@dmoody256 dmoody256 force-pushed the dmoody256:mingw_link_issue branch from b3f363b to 88621c3 Feb 8, 2019

@dmoody256 dmoody256 changed the title [WIP] Mingw link issue Mingw link issue Feb 8, 2019

@bdbaddog

This comment has been minimized.

Copy link
Contributor

commented Feb 15, 2019

Merging.

@bdbaddog bdbaddog merged commit f296600 into SCons:master Feb 15, 2019

5 of 6 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
Sider No issues left; 2 issues fixed.
Details
WIP Ready for review
Details
codecov/patch 100% of diff hit (target 100%)
Details
codecov/project 87.81% (+<.01%) compared to 9f09013
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.