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

🐛 Allow passing extra_libs to CheckLibWithHeader #4676

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

rdbisme
Copy link

@rdbisme rdbisme commented Jan 26, 2025

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
Copy link
Collaborator

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
Copy link
Author

rdbisme commented Jan 26, 2025

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

@bdbaddog
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Author

rdbisme commented Jan 26, 2025

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
Copy link
Author

rdbisme commented Jan 26, 2025

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

@mwichmann
Copy link
Collaborator

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.

@@ -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.
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants