Habemus Macam #82

jacg · 2023-08-16T23:46:25Z

Closes #15.

The remaining problems against which we were banging our heads recently, were caused by linking multiple versions of libc++, which was caused by the dependencies (G4 and Qt) and the client code using different compiler configurations.

The solution¹ is to use the gcc environment rather than the clang one in the flake's devShell. This is only necessary on Darwin, but (for now) it has been changed on Linux too.

The change to gcc breaks the compile-time tests we added recently (which have been removed, to make GHA pass).

Thus, before we can merge this, we have to:

Fix failing compile-time tests
Revert to clang on Linux

@soleti this has been tested on macOS on Intel, but we do not have access to Apple Silicon, so it would be good if you could check that it works there too.

(Why is the macOS GHA so slow? The one I've been waiting for before submitting this PR, completed on Linux in under 9 minutes, the macOS one chugged along for muuuch longer and then failed spuriously, so I had to restart it. Then it took another 21 minutes.)

This solution is temporary: a fix is in the pipeline upstream. ↩

soleti · 2023-08-17T07:59:15Z

This looks like it's working on macOS Silicon as well, I am able to run g4-examples/B1.

jacg · 2023-08-17T08:03:49Z

@soleti Could you also try the version in the fix-clang-on-darwin branch?

soleti · 2023-08-17T08:07:30Z

Also works.

jacg · 2023-08-17T08:32:29Z

This branch uses clang on Linux, and stdenv (g++) on Darwin. Over in fix-clang-on-darwin we have a version which uses clang on both, but forces the Darwin one to use the same version of libc++ as everything else. The latter is a bit ugly, and should become obsolete once the aforementioned upstream fix lands.

Which one do we prefer?

(I'll add the fix-clang-on-darwin solution here, for archival purposes: we can revert it if we want to stick with what is already here.)

fix-clang-on-darwin should sidestep the compile-time test failures on g++, but maybe we want to take this opportunity to address those. But I wouldn't delay merging this for the sake of fixing compile-time tests on g++.

g++ also spews an absurd amount of warnings.

There is also #17.

jacg · 2023-08-17T08:37:11Z

fix-clang-on-darwin should sidestep the compile-time test failures on g++

Apparently, not.

gonzaponte · 2023-08-17T08:38:08Z

Which one do we prefer?

Will it still be ugly once the upstream fix is in place?

There is one advantage of keeping g++ on darwin and that's that we don't ignore g++, which we have been doing for a while. As you say:

There is also #17.

But I wouldn't delay merging this for the sake of fixing compile-time tests on g++.

When I get some time I will explore what's going on with that in a separate branch

g++ also spews an absurd amount of warnings.

I guess this has to do with the different interpretations of -Wshadow and so on? If so we will fix it with pragmas like we did before.

jacg · 2023-08-17T08:57:47Z

Will it still be ugly once the upstream fix is in place?

Once the upstream fix is in place, it should look exactly like it did before we touched anything, but now it should work.

Currently the clang 16 env on Darwin uses a different version of libc++ from everything else, that's what was causing our problems. After the fix, it should use the same version. IIUC. So our old flake should Just Work.

There is one advantage of keeping g++ on darwin and that's that we don't ignore g++, which we have been doing for a while. As you say:

There is also #17.

I guess we should just add it to GHA.

Off the top of my head, it should be possible to have a default devShell with clang, and a second one with gcc. Then it should be possible to invoke the latter explicitly with something like nix develop .#gcc.

jacg · 2023-08-17T10:43:03Z

fix-clang-on-darwin should sidestep the compile-time test failures on g++

Apparently, not.

Fixed.

I've left the gcc-on-darwin-clang-on-linux solution in the history, for reference, and replaced it with the patch-clang-on-darwin-to-use-libc++-11 approach.

This avoids using g++, so the compile-time tests now pass.

I suggest that we merge, and deal with g++ related issues (broken compile-time tests, running g++ on GHA) in a separate PR.

gonzaponte · 2023-08-17T10:44:37Z

client_side_tests/client_fetch_content/CMakeLists.txt

+if(APPLE)
+    target_link_options(Nain4 PRIVATE -undefined dynamic_lookup)
+endif()
+
+if(APPLE)
+    target_link_options(client_exe PRIVATE -undefined dynamic_lookup)
+endif()


why is Nain4 a target here? this is a client, shouldn't need to mess with Nain4's link options.

I don't know why it's needed, but here is the proof that it is needed.

Ah, I think we go back again to the issue where fetchcontent grabs the last tag, not the current branch.
#67

Can we have a commit changing GIT_TAG to your branch to convince ourselves that it's the only issue?

So we'd have to add the if-apple-then-dynamic-lookup gubbins somewhere in there?

Yes, that absolutely needs to go in.

Anything else?

dealing with stupid macos is not a nain4 problem

Unfortunately, in the harsh Real World, it is very much our problem.

I can't think of anything else. Just remember to revert 9d9d552.

We can provide a CMake function for our clients to do if-apple-then-dynamic-lookup more conveniently, but that will probably not work out of the box and deserves its own PR.

The macos build is passing the tests. This means that the networkless recompilation test is not working as expected. My guess is that the command we use to restrict network usage is not working on macos. This means that it's allowed to use the network, fetch the branch and recompile, thus passing the test. But this is not supposed to happen.

And, I can confirm that this is the issue:

sudo: iptables: command not found

client_side_tests/client_subdirectory/CMakeLists.txt

flake.nix

jacg · 2023-08-17T15:36:11Z

Do you want to review #86 and/or #84 before I merge them?

I'm bored of having to cancel a duplicate workflow with every push, so it would be good to get at least #86 out of the way quickly.

gonzaponte · 2023-08-17T16:30:45Z

There seems to be a linking problem in macOS now?

jacg · 2023-08-17T16:39:11Z

Yes, we're going around in circles. Let's summarize:

I needed to add the dynamic-link stuff on apple in the client-side-tests, to make GHA pass
You say that this shouldn't be needed (makes sense).
But it is needed on this branch, because ... oh, I forget the details ... but it's related to Find tag automatically in client-side test #67
I've removed (some (the rest is stuck in an unpushable-until-I-know-more rebase)) of the dynamic-link stuff on apple ...
... so the workflows fail again ...
... which is why I said that (elsewhere) that the workflows won't be able to tell us anything sensible about the status of this PR once the apple dynamic linking stuff has been removed.

gonzaponte · 2023-08-17T16:44:42Z

But it is needed

I see two options:

Now that we understand the failures, we remove the offending lines, merge this PR and tag the new master
We merge this PR with the offending lines, tag master at that point and push another commit removing the lines.

I don't have a strong preference.

jacg · 2023-08-17T16:49:19Z

What do you consider to be the offending lines? I don't think there are any (unless you're referring to the apple dynamic links in the client-side tests, whose removal I have finally managed to push, so they are not there any more).

If I've got that right, then we

change the tag in the client-side test
merge
tag

done.

gonzaponte · 2023-08-17T16:51:47Z

unless you're referring to the apple dynamic links in the client-side tests,

Yes, I was referring to that. I'm not very focused today.

Your plan works. Let's go ahead.

jacg · 2023-08-17T16:54:12Z

You or me? I can do it, if you haven't started yet.

gonzaponte · 2023-08-17T16:55:09Z

Go for it.

jacg added 6 commits August 17, 2023 11:06

Replace clang with gcc in devShell

1db40a1

Disallow MacOS failures on GHA

cbd5c4d

Add target_link_options(Nain4 PRIVATE -undefined dynamic_lookup)

ff2b46f

Add target_link_options(nain4-tests PRIVATE -undefined dynamic_lookup)

fa329a6

Use -undefined dynamic_lookup only on APPLE

f5485ed

Remove clang{16,_tools} from devShell.packages

91ba475

jacg force-pushed the fix-darwin branch 3 times, most recently from 9daac46 to 8d6d851 Compare August 17, 2023 10:40

jacg marked this pull request as ready for review August 17, 2023 10:42

gonzaponte reviewed Aug 17, 2023

View reviewed changes

jacg mentioned this pull request Aug 17, 2023

Gcc devshell #84

Merged

jacg and others added 8 commits August 17, 2023 18:17

REVERTME: disable compile-time tests on GHA

b24bd88

Factor packages definition out of mkShell

c6215ae

Clang on Linux, stdenv on Darwin

5b6e5a9

Revert previous commit before alternative solution

cf356ba

Use libc++ 11 with clang 16 on Darwin

57c718b

Re-enable compile-time tests on GHA

c148aee

Change GIT_TAG: v0.1.8 -> origin/fix-darwin

2fb8b4f

Make sure failing commands don't go unnoticed

a5f796f

Revert to using tag v0.1.8 in client_fetch_content

61b7ae5

jacg force-pushed the fix-darwin branch from d85fc95 to 61b7ae5 Compare August 17, 2023 16:42

gonzaponte approved these changes Aug 17, 2023

View reviewed changes

jacg mentioned this pull request Aug 17, 2023

Document apple dynamic linking solution #87

Closed

jacg merged commit 61b7ae5 into master Aug 17, 2023
0 of 4 checks passed

jacg deleted the fix-darwin branch August 17, 2023 17:13

gonzaponte mentioned this pull request Aug 17, 2023

Fix test-fetch-content-recompile in macOS #88

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Habemus Macam #82

Habemus Macam #82

jacg commented Aug 16, 2023 •

edited

Loading

soleti commented Aug 17, 2023

jacg commented Aug 17, 2023

soleti commented Aug 17, 2023

jacg commented Aug 17, 2023

jacg commented Aug 17, 2023

gonzaponte commented Aug 17, 2023

jacg commented Aug 17, 2023

jacg commented Aug 17, 2023

gonzaponte Aug 17, 2023 •

edited

Loading

jacg Aug 17, 2023

gonzaponte Aug 17, 2023

jacg Aug 17, 2023

gonzaponte Aug 17, 2023

jacg Aug 17, 2023

jacg Aug 17, 2023

gonzaponte Aug 17, 2023

gonzaponte Aug 17, 2023

gonzaponte Aug 17, 2023

jacg commented Aug 17, 2023

gonzaponte commented Aug 17, 2023

jacg commented Aug 17, 2023

gonzaponte commented Aug 17, 2023

jacg commented Aug 17, 2023

gonzaponte commented Aug 17, 2023

jacg commented Aug 17, 2023

gonzaponte commented Aug 17, 2023

Habemus Macam #82

Habemus Macam #82

Conversation

jacg commented Aug 16, 2023 • edited Loading

Footnotes

soleti commented Aug 17, 2023

jacg commented Aug 17, 2023

soleti commented Aug 17, 2023

jacg commented Aug 17, 2023

jacg commented Aug 17, 2023

gonzaponte commented Aug 17, 2023

jacg commented Aug 17, 2023

jacg commented Aug 17, 2023

gonzaponte Aug 17, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jacg commented Aug 17, 2023

gonzaponte commented Aug 17, 2023

jacg commented Aug 17, 2023

gonzaponte commented Aug 17, 2023

jacg commented Aug 17, 2023

gonzaponte commented Aug 17, 2023

jacg commented Aug 17, 2023

gonzaponte commented Aug 17, 2023

jacg commented Aug 16, 2023 •

edited

Loading

gonzaponte Aug 17, 2023 •

edited

Loading