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.

Sign up for GitHub

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
Merged

Mingw link issue #3270

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
fdd3664
fixed mingw emitter to convert str to node before accessing node memb…
dmoody256 Jan 14, 2019
4518d14
updated CHANGES.txt
dmoody256 Jan 14, 2019
b68e505
check to make sure that the CCFLAGS are a CLVar
dmoody256 Jan 30, 2019
dd340d5
Merge remote-tracking branch 'origin/master' into mingw_link_issue
dmoody256 Jan 30, 2019
3341dc6
Merge remote-tracking branch 'origin/master' into mingw_link_issue
dmoody256 Feb 2, 2019
3454ffa
switch to 32 bit mingw in appveyor and add mingw env test
dmoody256 Feb 8, 2019
88621c3
Merge remote-tracking branch 'origin/master' into mingw_link_issue
dmoody256 Feb 8, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions src/CHANGES.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,8 @@ RELEASE VERSION/DATE TO BE FILLED IN LATER

From Daniel Moody:
- Update TempFileMunge class to use PRINT_CMD_LINE_FUNC
- Update link tool to convert target to node before accessing node member
- Update mingw tool to remove MSVC like nologo CCFLAG

RELEASE 3.0.3 - Mon, 07 Jan 2019 20:05:22 -0400
NOTE: 3.0.2 release was dropped because there was a packaging bug. Please consider all 3.0.2
Expand Down
2 changes: 2 additions & 0 deletions src/engine/SCons/Tool/link.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Curious why this started failing now?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@dmoody256 dmoody256 Feb 6, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@dmoody256 dmoody256 Feb 8, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

scons/src/engine/SCons/Tool/mslink.py

Lines 144 to 154 in 9f09013

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:

scons/src/engine/SCons/Tool/cyglink.py

Lines 100 to 106 in 9f09013

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

tgt = env.File(tgt)
tgt.attributes.shared = 1

try:
Expand Down
3 changes: 2 additions & 1 deletion src/engine/SCons/Tool/mingw.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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', ''))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

env['RC'] = 'windres'
env['RCFLAGS'] = SCons.Util.CLVar('')
env['RCINCFLAGS'] = '$( ${_concat(RCINCPREFIX, CPPPATH, RCINCSUFFIX, __env__, RDirs, TARGET, SOURCE)} $)'
Expand Down