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

support: allow override of ADAFLAGS, LDFLAGS, library soversion #585

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

asarhaddon
Copy link
Contributor

All this would be useful when packaging the support library for Debian.
I have tried to copy the code from gnatcoll so that it also benefits windows and macos.
Thanks for considering.

@CLAassistant
Copy link

CLAassistant commented Nov 16, 2021

CLA assistant check
All committers have signed the CLA.

@pmderodat
Copy link
Member

Thank you for your contributions! In order to merge them, we need you to sign the CLA.

Regarding the commit messages: could you keep them wrapped at 76 columns?

@@ -12,6 +12,14 @@ library project Langkit_Support is
Library_Kind_Param : Library_Kind_Type := external
("LIBRARY_TYPE", external ("LANGKIT_SUPPORT_LIBRARY_TYPE", "static"));

type OS_Kind is ("windows", "unix", "osx");
OS : OS_Kind := External ("LANGKIT_SUPPORT_OS", "windows");
Copy link
Member

Choose a reason for hiding this comment

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

We would like to keep things working with default options: please replace "windows" with "" here and adjust the rest of the patch to avoid defining Library_Version in that case?

Copy link
Member

Choose a reason for hiding this comment

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

Thank you. Unfortunately, this does not work:

$ gprbuild -Psupport/langkit_support.gpr
langkit_support.gpr:16:04: no value defined for "os"
gprbuild: "support/langkit_support.gpr" processing failed

I suppose this is a bug in GPRbuild, sorry I didn’t check earlier. To workaround it, I would suggest to replace "" with "unknown" or similar.

support/langkit_support.gpr Outdated Show resolved Hide resolved
support/langkit_support.gpr Outdated Show resolved Hide resolved
@raph-amiard
Copy link
Member

@asarhaddon ping! We would love to merge this, but you need to answer @pmderodat 's last comment

@asarhaddon
Copy link
Contributor Author

Hello.
I had a more simple idea meanwhile.
Instead of concatenating a suffix depending on LANGKIT_SUPPORT_OS and LANGKIT_SUPPORT_SOVERSION into Library_Version, we should maybe support a single LANGKIT_SONAME variable.
If the variable is unset, Library_Version remains unset in order to keep the existing behaviour. If it is set, the content is assigned to Library_Version.
Both usage and implementation would be simplified.
Should I implement this idea or an "unknown" choice for LANGKIT_SUPPORT_OS?

@pmderodat
Copy link
Member

[…] we should maybe support a single LANGKIT_SONAME variable. If the variable is unset, Library_Version remains unset in order to keep the existing behaviour. If it is set, the content is assigned to Library_Version

This looks like a good idea. Yes, let’s do it, thank you!

@pmderodat
Copy link
Member

Hello @asarhaddon, and sorry for taking so much time to get back to you.

The patch looks almost good to me: could you just rename the LANGKIT_SONAME external to LANGKIT_SUPPORT_SONAME?

Thank you in advance,

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.

4 participants