Add LIBLITERALPREFIX to support gcc -l:filename

[mwichmann]

This is done in a general way, using a construction var, although at the moment only the GNU linker is known to handle things this way.

Had to do something funky, or it won't work when os.pathsep and LIBLITRALPREFIX are the same, as they are on Linux (i.e. ':'). That's because SCons.PathList.PathList is called and it does:

class _Pathlist:
    def __init__(self, pathlist) -> None:
        if SCons.Util.is_String(pathlist):
            pathlist = pathlist.split(os.pathsep)

Splitting has the effect of turning :libm.a into ((0, ''), (0, 'libm.a')] That is, the ':' is lost as part of the library specifier - so need to try to avoid that.

Fixes #3951

Submitted for initial review so no docs or changelogs yet.

Contributor Checklist:

• I have created a new test or updated the unit tests to cover the new/changed functionality.
• I have updated CHANGES.txt (and read the README.rst)
• I have updated the appropriate documentation

[bdbaddog]

Would be better to add another parameter to _stripixes() instead of having an envar LIBLITERAL.. That way any users usage of _stripixes() won't be affected by LIBLITERAL. ?

[mwichmann]

Not sure that I follow the benefits of that. Somewhere, these things need to happen:

• Need to inform SCons of a compiler-specific (actually, linker-specific) bit of syntax, which may be empty. Construction vars are the way SCons normally does that.
• That syntax needs to be conveyed to _stripixes so it doesn't transform libfoo.a to foo.
• If _stripixes calls PathList (which it does currently), the _PathList initializer needs to be told not to strip the linker syntax from the arguments, if it might do that. Which for the GNU linker on non-WIndows, it would, since the same character is used for two purposes. PathList is called a bunch of places, it's a little odd that it's called with a list of library names, but things broke if I had it stop doing that, didn't research why in any great depth.

Doing this as an argument to _stripixes means we need to be able to pass that in when needed, but we don't ever call _stripixes directly, it's only embedded in macro-like consvars which will be expanded at build time (currently _LIBFLAGS and _DLIBFLAGS). So you'd have to change the definition of those, which seems just as (or even more) fragile, as users are allowed to define their own _LIBFLAGS and wouldn't know they might need to change those definitions. And to do the subtitution when _LIBFLAGS is evaluated to pass through that magic indicator, you pretty much still need a consvar.

[bdbaddog]

Not sure that I follow the benefits of that. Somewhere, these things need to happen:

• Need to inform SCons of a compiler-specific (actually, linker-specific) bit of syntax, which may be empty. Construction vars are the way SCons normally does that.
• That syntax needs to be conveyed to _stripixes so it doesn't transform libfoo.a to foo.
• If _stripixes calls PathList (which it does currently), the _PathList initializer needs to be told not to strip the linker syntax from the arguments, if it might do that. Which for the GNU linker on non-WIndows, it would, since the same character is used for two purposes. PathList is called a bunch of places, it's a little odd that it's called with a list of library names, but things broke if I had it stop doing that, didn't research why in any great depth.

Doing this as an argument to _stripixes means we need to be able to pass that in when needed, but we don't ever call _stripixes directly, it's only embedded in macro-like consvars which will be expanded at build time (currently _LIBFLAGS and _DLIBFLAGS). So you'd have to change the definition of those, which seems just as (or even more) fragile, as users are allowed to define their own _LIBFLAGS and wouldn't know they might need to change those definitions. And to do the subtitution when _LIBFLAGS is evaluated to pass through that magic indicator, you pretty much still need a consvar.

Disagree.. You can note to users that another argument is available, but default it in the arglist to allow it to not be passed.
Then there's no magic happening.

Otherwise if this function is used for purposes other than the one's SCons itself is, there may be unpleasant surprises..

I'd like to avoid those surprises..

[bdbaddog]

Ok. Finally figured out why this solution was bugging me (besides already stated reasons).

Since LIBLITERAL's not on the command line in any way, if you change it's value, you won't get a rebuild.
Thus the justification for adding it as an optional variable to _stripixes

[mwichmann]

doesn't make any sense to change it, it's internal to the linker. if you change the linker to get different value, you will get rebuilds, since binary's sig will be different (hypothetical at moment)

only sane way to do this is add a new library holder type so libs can have attributes, but that's a lot more work, and api impact.

[mwichmann]

Since LIBLITERAL's not on the command line in any way, if you change it's value, you won't get a rebuild.

Not completely sure I understand this part of the comment, but perhaps a small sample will illustrate:

$ cat SConstruct
env = Environment(LIBLITERAL=":")
env.Program(target="hello", source="hello.c", LIBS=":libm.a")
$ scons -Q
gcc -o hello.o -c hello.c
gcc -o hello hello.o -l:libm.a
$ scons -Q
scons: `.' is up to date.
$ #EDIT SConstruct
$ cat SConstruct
env = Environment(LIBLITERAL="+")
env.Program(target="hello", source="hello.c", LIBS="+libm.a")
$ scons -Qn
gcc -o hello hello.o -l+libm.a
$

We've first set it to a :, and since the PR changes mean it's not stripped, it does pass through to the command line. Then change it to + after a build is previously shown as up to date, and it does force a rebuild, and you can see the + present in proposed command for the subsequent run (scons -n since the new literal-introducer of course wouldn't work, since it's unknown to ld).

[mwichmann]

Hmmm, merged the PR which seemed okay, but now it's failing tests, specifically test/ninja/build_libraries.py in the "experimental" set, and a whole bunch more in the full run. @bdbaddog are you able to get a quick look at that? Else I'll swing back to it later.

[mwichmann]

Okay, the failure comes down to - when _stripixes is handed a string it goes through a call chain that ends up making a StringSubber instance, and then calling its expand method. That does:

     try:
          s = eval(key, self.gvars, lvars)
     except KeyboardInterrupt:
          raise
     except Exception as e:
          if e.__class__ in AllowableExceptions:
               return ''
          raise_exception(e, lvars['TARGETS'], old_s)

The eval takes a NameError when LIBLITERAL is in the string to be expanded, but is not in self.gvars or in lvars. self.gvars is populated from the construction environment. The NameError is one of the AllowableExceptions, so instead of returning failure here, we return an empty string. That would be fine if we were just subbing LIBLITERAL, but instead we're subbing a string-that-looks-like-a-function-call, so having the thing collapse down to an empty string is bad.

The conclusion I make is if we're going to use LIBLITERAL in an expansion like this, it must be defined in the environment. I experiment with initializing it in the platform modules for posix and win32, and that makes the test suite go back to running normally instead of taking 40-ish failures.

[bdbaddog]

Annotate with gh defect this fixes? (Same for RELEASE)?

[bdbaddog]

Does this handle when LIBLITERALPREFIX isn't defined now? Or did you just define this down all the likely paths?

[mwichmann]

paths (in platform). I don't know how to fix otherwise, eval just has to have stuff in one of the two dicts, and don't see another clean way

[bdbaddog]

@mwichmann Should we also set LIBLITERALPREFIX to : in the gnulink tool?