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

cmake: add the -dynamic-linker=... form to the -dynamic-linker regex #22308

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

Conversation

michoecho
Copy link
Contributor

On my system (Nix), the compiler produces a -dynamic-linker=/nix/store/... in the linker call scanned by get_padded_dynamic_linker_option. But the regex can't deal with the = there, it requires a . Fix that.

Could be backported, but it's not needed for anything.

@michoecho michoecho requested a review from tchaikov January 14, 2025 11:54
@michoecho michoecho added the backport/none Backport is not required label Jan 14, 2025
Copy link
Contributor

@tchaikov tchaikov left a comment

Choose a reason for hiding this comment

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

lgtm. but shall we apply a similar change to configure.py? as it is using

original_dynamic_linker = re.search('-dynamic-linker ([^ ]*)', gcc_linker_output).groups()[0]
. something like

diff --git a/configure.py b/configure.py
index 4610030970..5bc7a9c8c0 100755
--- a/configure.py
+++ b/configure.py
@@ -1702,7 +1702,7 @@ def generate_version(date_stamp):
 # the program headers.
 def dynamic_linker_option():
     gcc_linker_output = subprocess.check_output(['gcc', '-###', '/dev/null', '-o', 't'], stderr=subprocess.STDOUT).decode('utf-8')
-    original_dynamic_linker = re.search('-dynamic-linker ([^ ]*)', gcc_linker_output).groups()[0]
+    original_dynamic_linker = re.search('-dynamic-linker[ =]([^ ]*)', gcc_linker_output).groups()[0]
 
     employ_ld_trickery = True
     # distro-specific setup

or it is immune to this problem?

On my system (Nix), the compiler produces a `-dynamic-linker=/nix/store/...` in
the linker call scanned by get_padded_dynamic_linker_option.
But the regex can't deal with the `=` there, it requires a ` `. Fix that.

We also do the same in configure.py, and remove the Nix-specific hack
which used to disable the entire mechanism.
@michoecho michoecho force-pushed the cmake_dynamic_linker_equals branch from 80f46b1 to 66d5a82 Compare January 14, 2025 13:02
@michoecho
Copy link
Contributor Author

lgtm. but shall we apply a similar change to configure.py? as it is using

original_dynamic_linker = re.search('-dynamic-linker ([^ ]*)', gcc_linker_output).groups()[0]

. something like

diff --git a/configure.py b/configure.py
index 4610030970..5bc7a9c8c0 100755
--- a/configure.py
+++ b/configure.py
@@ -1702,7 +1702,7 @@ def generate_version(date_stamp):
 # the program headers.
 def dynamic_linker_option():
     gcc_linker_output = subprocess.check_output(['gcc', '-###', '/dev/null', '-o', 't'], stderr=subprocess.STDOUT).decode('utf-8')
-    original_dynamic_linker = re.search('-dynamic-linker ([^ ]*)', gcc_linker_output).groups()[0]
+    original_dynamic_linker = re.search('-dynamic-linker[ =]([^ ]*)', gcc_linker_output).groups()[0]
 
     employ_ld_trickery = True
     # distro-specific setup

or it is immune to this problem?

If you scroll down just 2 lines from there, you will see that someone added a Nix-specific hack which completely disables the -dynamic-linker overwrite on Nix. So it's not that it's immune to the problem, someone just hacked around it :P

I updated the patch with a corresponding change to the regex in configure.py, and I removed the Nix-specific hack.

@michoecho
Copy link
Contributor Author

Hopefully it doesn't break some other piece of the build on Nix. But it works for me, and I think I'm probably the only person who builds Scylla on Nix right now.

@michoecho
Copy link
Contributor Author

The Nix build scripts in the Scylla repository are unmaintained and broken anyway, so I doubt even more that anyone other than me builds Scylla on Nix. I don't use those scripts, I have my own private ones.

@scylladb-promoter
Copy link
Contributor

🔴 CI State: FAILURE

✅ - Build
✅ - dtest
✅ - dtest with topology changes
✅ - dtest with tablets
❌ - Unit Tests

Build Details:

  • Duration: 10 hr
  • Builder: i-081858fb74ecdd529 (m5ad.8xlarge)

@tchaikov
Copy link
Contributor

🔴 CI State: FAILURE

✅ - Build ✅ - dtest ✅ - dtest with topology changes ✅ - dtest with tablets ❌ - Unit Tests

Build Details:

* Duration: 10 hr

* Builder: i-081858fb74ecdd529 (m5ad.8xlarge)
a boost test failed: test/boost/Debug/combined_tests --run_test=schema_change_test/test_schema_digest_does_not_change_without_digest_feature 

#22243

@scylladb-promoter
Copy link
Contributor

🔴 CI State: FAILURE

✅ - Build
✅ - dtest with topology changes
✅ - dtest with tablets
✅ - dtest
❌ - Unit Tests

Build Details:

  • Duration: 10 hr
  • Builder: i-08e8992f2054d91d4 (m5ad.8xlarge)

@scylladb-promoter
Copy link
Contributor

🔴 CI State: FAILURE

✅ - Build
✅ - dtest with topology changes
✅ - dtest
✅ - dtest with tablets
❌ - Unit Tests

Build Details:

  • Duration: 10 hr
  • Builder: i-04c428f99c30ceae6 (m5ad.8xlarge)

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

Successfully merging this pull request may close these issues.

4 participants