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

does macos building work? #134

Merged
merged 19 commits into from
Dec 16, 2022
Merged

does macos building work? #134

merged 19 commits into from
Dec 16, 2022

Conversation

lgray
Copy link
Collaborator

@lgray lgray commented Nov 4, 2022

No description provided.

@henryiii
Copy link
Member

henryiii commented Nov 4, 2022

configure:4562: error: C compiler cannot create executables

This is trying to build for GCC on clang, I think?

@lgray
Copy link
Collaborator Author

lgray commented Nov 4, 2022

macos should have the gcc frontend for llvm installed as well.

@chrispap95
Copy link
Collaborator

chrispap95 commented Nov 4, 2022

Hi @lgray, I also gave a shot at this yesterday but I didn't get it to work. I was actually able to get it to compile with clang by replacing -rpath=$$ORIGIN -> -rpath,$$ORIGIN. GNU build for Linux didn't have trouble building as well. I guess building them all with GCC might be more consistent though? I don't have a particular opinion as far as it works. People more familiar with compilers may weigh in. Another problem is these make commands during the installation step. I had to switch to gmake. Again, no issues with the Linux dist. Still, there are two remaining issues:

  1. these make commands still don't give the expected outputs
  2. there seems to be confusion with macOS versions. This thing is running on macos11 but there are directories having 10.9 and 10.15 as well. I think the linker is also confused by this.

Unfortunately, I won't have time to troubleshoot this more (today at least).

@chrispap95
Copy link
Collaborator

Actually, I almost got this to work: https://github.com/chrispap95/fastjet/actions/runs/3392170281/jobs/5638053408
Only one of the tests fails for some reason.

@lgray
Copy link
Collaborator Author

lgray commented Nov 4, 2022

This is a nice to have not a need to have so we can take care of this one with some leisure.

@chrispap95
Copy link
Collaborator

This morning I found some time to investigate this a bit more. The status is the following:

  • clang can configure and build fastjet without CGAL.
  • clang cannot configure when CGAL is turned on. It seems that CGAL needs some C++17 features.
  • clang with -stdc++17 and CGAL configures successfully but the build fails because auto_ptr is completely deprecated in C++17 implementation of clang.

I believe that we have two options:

  1. Wait for FastJet 3.4.1 to supply MacOS wheels. I know that auto_ptrs were removed from the master branch of fastjet-core. I didn't try to build it yet but I assume that this incompatibility will be resolved by then.
  2. Use GCC instead. GCC didn't depreciate completely auto_ptr in C++17. The build proceeds past that point and fails due to a linking issue later. We will have to resolve that. Also, we have to pass a lot of extra options to make this work. I had to set CC, CXX, CXXFLAGS flags and pass manually the locations to boost, mpfr, and gmp. It's a mess...

@chrispap95
Copy link
Collaborator

Current status: everything builds & tests successfully except for test_007-general.py::test_func_call__input on macOS in which get_parents() seems to return a garbage awkward array.

@henryiii
Copy link
Member

henryiii commented Nov 22, 2022

You should not use gcc on macOS. That will create a mass of problems. You can (and should) set MACOSX_DEPLOYMENT_TARGET to something that supports C++17 (10.12-10.14, depending on how many C++17 features you use).

(Edit: Ahh, didn't real all the messages, okay).

@jmduarte
Copy link
Collaborator

just tagging @jpata who was interested in this as well. @chrispap95 is there anything in particular that others can help with to move this forward?

@chrispap95
Copy link
Collaborator

So now this fails for the same reason #153 fails. Between, those 2 cibuildwheel versions it seems that the docker image changes. Since this problem now propagates to MacOS, I am wondering whether this is caused by some dependency update (e.g. swig). I just checked the two images and indeed swig goes from 4.0.2 to 4.1.0. MacOS has 4.1.1 . Not sure if this is the problem though and I might not have time to look into it till next week.

Now, for MacOS, apart from this new issue which seems universal, I had it almost working. Only the very last check was failing for some strange reason and I never got to debug it. It seems that we have to solve the issue with ClusterSequence_fastjet_banner_stream before attempting to fix the build for MacOS. :(

@lgray
Copy link
Collaborator Author

lgray commented Dec 15, 2022

fastjet will compile with swig back to version 2. The underscores to dots problem is recent v4 issue. If we can pin to an older swig in the package managers we can get around it (this is usually possible).

@chrispap95
Copy link
Collaborator

Actually, the problem here is those @staticmethod's. I will open a PR. The fix is straightforward.

@lgray
Copy link
Collaborator Author

lgray commented Dec 16, 2022

Oh, weird. Kind of surprised swig would change the behavior of something like that. Good find. :-)

@chrispap95
Copy link
Collaborator

chrispap95 commented Dec 16, 2022

oh, that's interesting. I guess now we can have working MacOS wheels!

@jmduarte
Copy link
Collaborator

@chrispap95 that's great! Is there anything left to do for this PR?

@chrispap95
Copy link
Collaborator

@jmduarte not much. I will push a commit to build wheels as well for MacOS and if this is also successful then we can merge and publish.

@chrispap95
Copy link
Collaborator

Interesting. This single failure is similar to the old failure (before awkward 2) but not exactly the same. The same test fails but for different reasons. The old test was failing because the offset array was garbage. This time part of the result is empty???

@chrispap95
Copy link
Collaborator

It's done. It seems that I forgot to remove 2 of those dangerous reallocs, that I inserted into the code, and they were causing the offset array to pick up noise. We should be able to publish MacOS wheels after merging this + methods to_numpy_get_parents and to_numpy_get_childare now memory safe.

@jmduarte jmduarte self-requested a review December 16, 2022 23:21
Copy link
Collaborator

@jmduarte jmduarte left a comment

Choose a reason for hiding this comment

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

Awesome, this looks great! I just tested it on my MacBook Pro with macOS 13.0.1, python 3.11.0 and the generated wheel worked perfectly out of the box!

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.

None yet

4 participants