🐛 Allow passing `extra_libs` to `CheckLibWithHeader` #4676

rdbisme · 2025-01-26T18:31:28Z

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 RELEASE.txt (and read the README.rst).
I have updated the appropriate documentation

mwichmann · 2025-01-26T19:03:20Z

The concept seems good... not sure why that argument was left out of the upper layer when it's present in the lower layer (Conftest.py). Could ask the same question about SConf/CheckLib.

rdbisme · 2025-01-26T19:12:39Z

First time I contribute to scons. Do I need to add a specific test for this?

bdbaddog · 2025-01-26T21:09:12Z

First time I contribute to scons. Do I need to add a specific test for this?

Yes. That's why we have the checklist in the initial decription.
You can edit it and add an X next to each item once you've added it.

bdbaddog · 2025-01-26T21:10:02Z

And add this change to the docs as well.
We'll help you through those if you need help.
Also asking on the discord server may be useful.

rdbisme · 2025-01-26T22:06:25Z

Yes. That's why we have the checklist in the initial decription.
You can edit it and add an X next to each item once you've added it.

I mean, my remark was more like that test wouldn't test much? CheckLib already has that argument. I'm just exposing it in the interface of CheckLibWithHeadere, but I'll give a look to the tests. :)

rdbisme · 2025-01-26T23:37:23Z

I should have added all things required. I'm not 100% sure about the doc (XML, really?! :)).

mwichmann · 2025-01-27T02:26:16Z

Yes, xml, really :-) (this project dates back just a few years, like 20+). We can help with docs, there's not really much to do there for this kind of change.

The configure stuff is a weird two-layer thing, SConf is the "API layer" and Conftest is the implementation layer. Except, they reach into each other and aren't as separated as they could be. Not making excuses.

bdbaddog · 2025-01-27T05:03:42Z

doc/man/scons.xml

@@ -4244,6 +4244,7 @@ to serve as the test can be supplied in
 if not supplied,
 the default checks the ability to link against the specified
 <parameter>library</parameter>.
+<parameter>extra_libs</parameter> can be used to add additional libraries to link against.


@mwichmann - what's the blurb to indicate this is added in NEXT_RELEASE?

Since the xml files are not processed by Sphinx, it needs the hand-crafted version:

<para> <emphasis>Changed in version NEXT_RELEASE:</emphasis> Added the <parameter>extra_libs</parameter> parameter. </para>

It's a little unclear whether an existing method getting a new parameter is considered "added" or "changed". I'm tending to follow what CPython does, which is to consider that "changed" (though I admit that's not been consistent).

The corresponding markup in source code (aka the docstring) is:

.. versionchanged:: NEXT_RELEASE Added the *extra_libs* parameter.

mwichmann · 2025-01-27T14:48:24Z

Still think this should be added to SCons.SConf.CheckLib as well.

mwichmann · 2025-02-07T14:06:49Z

If I can figure out how to do it, I'm going to push an update to this that

also enables extra_libs for the CheckLib API,
adds the version-added rst to the docstrings
adds the version-added xml and the `extra_libs addition to the manpage, plus tweaks a little bit (trying to resist doing more this time!)

mwichmann · 2025-02-07T14:10:16Z

Hmmm, I just spotted that this isn't passing on the Windows CI:

 213/1250 (17.04%) C:\hostedtoolcache\windows\Python\3.12.8\x64\python.exe test\Configure\CheckLibWithHeader_extra_libs.py
D:\a\scons\scons\scripts\scons.py returned 2
STDOUT =========================================================================
scons: Reading SConscript files ...
scons: done reading SConscript files.
scons: Building targets ...
cl /FolibB.obj /c libB.c /nologo /IC:\Users\RUNNER~1\AppData\Local\Temp\scons\testcmd.5912.50_ghoo6\libA
libB.c
link /nologo /dll /out:libB.dll /implib:libB.lib /LIBPATH:C:\Users\RUNNER~1\AppData\Local\Temp\scons\testcmd.5912.50_ghoo6\libA A.lib libB.obj
LINK : fatal error LNK1181: cannot open input file 'A.lib'
scons: building terminated because of errors.

STDERR =========================================================================
scons: *** [libB.dll] Error 1181

Will investigate soon if someone doesn't get there first...

mwichmann · 2025-02-07T17:42:20Z

Need some guidance here... on Windows you have to link against the .lib version, which doesn't even build... not sure how this is supposed to work.

Manpage entries and docstrings updated with version-added specifiers. Signed-off-by: Mats Wichmann <[email protected]>

Add boilderplate header/footer, fix one place where the wrong (non-portable) name for a library was used. Still does not build on Windows, due to mismatch between .dll/.lib construction. Signed-off-by: Mats Wichmann <[email protected]>

"Muscle memory" had me add calls to skip tools for DefaultEnvironment, but the two library builds use it, so that was just plain wrong. Restored prior state. Signed-off-by: Mats Wichmann <[email protected]>

jcbrill · 2025-02-09T15:39:38Z

@mwichmann My guess is that A.dll does not export any symbols; therefore, an import library is not generated.

mwichmann · 2025-02-09T15:42:12Z

@mwichmann My guess is that A.dll does not export any symbols; therefore, an import library is not generated.

That's certainly true for the test file in the PR. I've added those in a worktree here, and it still doesn't, so I think I must be doing something wrong with that.

jcbrill · 2025-02-09T16:11:26Z

The following builds libA with MSVC from the command-line outside of the test. It correctly produces the A.lib file. I haven't checked to see if it breaks gnu/gcc yet. It shouldn't but one never knows.

libA/libA.h:

#ifdef _MSC_VER
  #ifdef BUILDINGSHAREDLIB
    #define MYAPI __declspec(dllexport)
  #else
    #define MYAPI __declspec(dllimport)
  #endif
#else
  #define MYAPI
#endif

MYAPI void libA();

libA/libA.c:

#include <stdio.h>
#include "libA.h"

MYAPI void libA() {
    printf("libA\\n");
}

libA/SConstruct:

SharedLibrary('A', source=['libA.c'], CPPDEFINES="BUILDINGSHAREDLIB")

I updated the test and it failed though.

P.S.: pretty sure the extra backslash in the print statement is erroneous.

mwichmann · 2025-02-09T16:18:19Z

@mwichmann My guess is that A.dll does not export any symbols; therefore, an import library is not generated.

That's certainly true for the test file in the PR. I've added those in a worktree here, and it still doesn't, so I think I must be doing something wrong with that.

I have an alternate independently developed example which does work, so later I'll reconcile the two and hopefully find what errors I made adjusting the original.

mwichmann · 2025-02-09T16:20:05Z

The following builds libA with MSVC from the command-line outside of the test. It correctly produces the A.lib file. I haven't checked to see if it breaks gnu/gcc yet. It shouldn't but one never knows.

libA/libA.h:
#ifdef _MSC_VER
  #ifdef BUILDINGSHAREDLIB
    #define MYAPI __declspec(dllexport)
  #else
    #define MYAPI __declspec(dllimport)
  #endif
#else
  #define MYAPI
#endif

MYAPI void libA();
libA/libA.c:
#include <stdio.h>
#include "libA.h"

MYAPI void libA() {
    printf("libA\\n");
}
libA/SConstruct:
SharedLibrary('A', source=['libA.c'], CPPDEFINES="BUILDINGSHAREDLIB")
I updated the test and it failed though.

P.S.: pretty sure the extra backslash in the print statement is erroneous.

yes, that looks like what it should want. And the backslash is left over from when it was a string-print in the testcase, once listed as raw-string, then not, then I think put back. Anyway, this should be enough to wrap it up.

jcbrill · 2025-02-09T17:49:08Z

Attached is a version of test/Configure/CheckLibWithHeader_extra_libs.py which appears to pass on Windows using msvc and WSL using gcc.

CheckLibWithHeader_extra_libs.zip

In hindsight, the double backslash may be necessary in the test since it is not inside a raw string (e.g., r""").

mwichmann · 2025-02-09T17:58:52Z

Attached is a version of test/Configure/CheckLibWithHeader_extra_libs.py which appears to pass on Windows using msvc and WSL using gcc.

CheckLibWithHeader_extra_libs.zip

In hindsight, the double backslash may be necessary in the test since it is not inside a raw string (e.g., r""").

thanks. I have one working here, but will look over.

Signed-off-by: Mats Wichmann <[email protected]>

mwichmann · 2025-02-09T18:27:48Z

Okay, this should be working now. Thanks to @jcbrill for confirming that what I was figuring out was on a workable path.

rdbisme · 2025-02-13T03:03:02Z

Thanks for bringing this forward :)

Signed-off-by: Mats Wichmann <[email protected]>

bdbaddog · 2025-02-15T17:37:51Z

test/Configure/CheckLibWithHeader_extra_libs.py

-SharedLibrary(target='A', source=['libA.c'], CPPDEFINES='BUILDINGSHAREDLIB')
-""",
-)
+test.dir_fixture(['fixture', 'checklib_extra', 'libA'], 'libA')


I think you should just be able to do this once as
test.dir_fixture(['fixture', 'checklib_extra'])

I suppose. If you think it's worth doing yet another iteration...

…ment(tools=[]) to speed up on windows

🐛 Allow passing extra_libs to CheckLibWithHeader

7575421

rdbisme mentioned this pull request Jan 26, 2025

One of the checks for sundials doesn't support sundials built with MPI support Cantera/cantera#1843

Open

rdbisme added 2 commits January 27, 2025 00:23

✅ Add test

dae3e49

📝 Add change summary

5c1ee17

rdbisme force-pushed the extra-libs branch from fb62c2e to 5c1ee17 Compare January 26, 2025 23:37

bdbaddog reviewed Jan 27, 2025

View reviewed changes

mwichmann added the Configure any issues related to Configure contexts label Jan 27, 2025

mwichmann added 3 commits February 7, 2025 12:13

Also enable extra_libs for CheckLib

2bb3cd3

Manpage entries and docstrings updated with version-added specifiers. Signed-off-by: Mats Wichmann <[email protected]>

Minor fixes to CheckLibWithHeader test

07e63ae

Add boilderplate header/footer, fix one place where the wrong (non-portable) name for a library was used. Still does not build on Windows, due to mismatch between .dll/.lib construction. Signed-off-by: Mats Wichmann <[email protected]>

Fix CheckLibWithHeader test - undo mistake

b1d71e6

"Muscle memory" had me add calls to skip tools for DefaultEnvironment, but the two library builds use it, so that was just plain wrong. Restored prior state. Signed-off-by: Mats Wichmann <[email protected]>

Windows-ify the new CheckLibWithHeader test

5f8a725

Signed-off-by: Mats Wichmann <[email protected]>

Per request, "fixture-ize" CheckLibWithHeader_extra_libs test

9bcbf7b

Signed-off-by: Mats Wichmann <[email protected]>

bdbaddog reviewed Feb 15, 2025

View reviewed changes

bdbaddog added 5 commits February 15, 2025 13:21

Simplify use of dir_fixture and some test logic. Added DefaultEnviron…

663165b

…ment(tools=[]) to speed up on windows

Simplify use of dir_fixture

2a9d19a

Simplify use of dir_fixture

a5fcde6

Simplify

e3c23de

Add note to docstring that this is a live test per mwichmann

769eff9

bdbaddog merged commit 2a85d9e into SCons:master Feb 15, 2025
6 of 8 checks passed

mwichmann added this to the 4.9 milestone Feb 15, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🐛 Allow passing `extra_libs` to `CheckLibWithHeader` #4676

🐛 Allow passing `extra_libs` to `CheckLibWithHeader` #4676

rdbisme commented Jan 26, 2025 •

edited

Loading

mwichmann commented Jan 26, 2025

rdbisme commented Jan 26, 2025

bdbaddog commented Jan 26, 2025

bdbaddog commented Jan 26, 2025

rdbisme commented Jan 26, 2025 •

edited

Loading

rdbisme commented Jan 26, 2025

mwichmann commented Jan 27, 2025

bdbaddog Jan 27, 2025

mwichmann Jan 27, 2025

mwichmann commented Jan 27, 2025

mwichmann commented Feb 7, 2025

mwichmann commented Feb 7, 2025

mwichmann commented Feb 7, 2025

jcbrill commented Feb 9, 2025

mwichmann commented Feb 9, 2025

jcbrill commented Feb 9, 2025

mwichmann commented Feb 9, 2025

mwichmann commented Feb 9, 2025

jcbrill commented Feb 9, 2025

mwichmann commented Feb 9, 2025

mwichmann commented Feb 9, 2025

rdbisme commented Feb 13, 2025

bdbaddog Feb 15, 2025

mwichmann Feb 15, 2025

🐛 Allow passing extra_libs to CheckLibWithHeader #4676

🐛 Allow passing extra_libs to CheckLibWithHeader #4676

Conversation

rdbisme commented Jan 26, 2025 • edited Loading

Contributor Checklist:

mwichmann commented Jan 26, 2025

rdbisme commented Jan 26, 2025

bdbaddog commented Jan 26, 2025

bdbaddog commented Jan 26, 2025

rdbisme commented Jan 26, 2025 • edited Loading

rdbisme commented Jan 26, 2025

mwichmann commented Jan 27, 2025

bdbaddog Jan 27, 2025

Choose a reason for hiding this comment

mwichmann Jan 27, 2025

Choose a reason for hiding this comment

mwichmann commented Jan 27, 2025

mwichmann commented Feb 7, 2025

mwichmann commented Feb 7, 2025

mwichmann commented Feb 7, 2025

jcbrill commented Feb 9, 2025

mwichmann commented Feb 9, 2025

jcbrill commented Feb 9, 2025

mwichmann commented Feb 9, 2025

mwichmann commented Feb 9, 2025

jcbrill commented Feb 9, 2025

mwichmann commented Feb 9, 2025

mwichmann commented Feb 9, 2025

rdbisme commented Feb 13, 2025

bdbaddog Feb 15, 2025

Choose a reason for hiding this comment

mwichmann Feb 15, 2025

Choose a reason for hiding this comment

🐛 Allow passing `extra_libs` to `CheckLibWithHeader` #4676

🐛 Allow passing `extra_libs` to `CheckLibWithHeader` #4676

rdbisme commented Jan 26, 2025 •

edited

Loading

rdbisme commented Jan 26, 2025 •

edited

Loading