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

Add Fastjet #21052

Merged
merged 56 commits into from
Feb 22, 2024
Merged

Add Fastjet #21052

merged 56 commits into from
Feb 22, 2024

Conversation

lgray
Copy link
Contributor

@lgray lgray commented Nov 7, 2022

Checklist

  • Title of this PR is meaningful: e.g. "Adding my_nifty_package", not "updated meta.yaml".
  • License file is packaged (see here for an example).
  • Source is from official source.
  • Package does not vendor other packages. (If a package uses the source of another package, they should be separate packages or the licenses of all packages need to be packaged).
  • If static libraries are linked in, the license of the static library is packaged.
  • Package does not ship static libraries. If static libraries are needed, follow CFEP-18.
  • Build number is 0.
  • A tarball (url) rather than a repo (e.g. git_url) is used in your recipe (see here for more details).
  • GitHub users listed in the maintainer section have posted a comment confirming they are willing to be listed there.
  • When in trouble, please check our knowledge base documentation before pinging a team.

@conda-forge-linter
Copy link

Hi! This is the friendly automated conda-forge-linting service.

I just wanted to let you know that I linted all conda-recipes in your PR (recipes/fastjet) and found it was in an excellent condition.

@lgray lgray changed the title Fastjet Add Fastjet Nov 7, 2022
@lgray
Copy link
Contributor Author

lgray commented Nov 7, 2022

@jpivarski @chrispap95 @jmduarte could you please give a thumbs-up here to indicate your willingness to be a maintainer?

@jpivarski
Copy link

I can be listed as a maintainer for the conda-forge recipe, just as I am for uproot and awkward, but I'm not actually familiar with how conda-forge recipes work. I can't be the only maintainer because if something goes wrong, I'll need to reach out for help.

But I'm happy to be in the loop about whether or not the conda-forge fastjet packages are being deployed correctly.

@lgray
Copy link
Contributor Author

lgray commented Nov 7, 2022

OK - so the problem appears to be that boost isn't being put into the include path in a reasonable way. It's being a bit stubborn. Perhaps @chrisburr has some idea how to patch it in.

@lgray
Copy link
Contributor Author

lgray commented Nov 7, 2022

  /home/conda/staged-recipes/build_artifacts/fastjet_1667857508475/work/CGAL-5.5.1/include/CGAL/config.h:127:10: fatal error: boost/config.hpp: No such file or directory
    127 | #include <boost/config.hpp>
        |          ^~~~~~~~~~~~~~~~~~

Yep - quite unsure what else to try to fiddle with. Boost doesn't seem to install correctly for the compiler to pick it up via conda (despite mpfr and gmp working fine).

@chrisburr
Copy link
Member

Why are you builing CGAL?

@chrispap95
Copy link

CGAL headers are needed to provide NlnN methods at very high multiplicities. We can build without CGAL but that would strip some functionality off fastjet.

@chrisburr
Copy link
Member

You should be able to get CGAL as a dependency rather than building it: https://github.com/conda-forge/cgal-cpp-feedstock

@chrispap95
Copy link

We are not building CGAL. We are only using the headers. Is this going to make the headers available?

@chrisburr
Copy link
Member

We are not building CGAL. We are only using the headers. Is this going to make the headers available?

Yes.

@chrispap95
Copy link

Okay, I see. Unfortunately, I don't have write access to the PR. I guess @lgray will try this later.

Comment on lines 19 to 24
missing_dso_whitelist:
- $RPATH/libfastjet.so.0
- $RPATH/libfastjettools.so.0
Copy link
Member

Choose a reason for hiding this comment

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

You probably shouldn't be using this, if the CI fails because of it there is probably a bug.

Copy link
Contributor Author

@lgray lgray Nov 8, 2022

Choose a reason for hiding this comment

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

That was your addition to the meta.yaml on the previous iteration of this pr. a9b9037 . There is very likely some bug or weird workaround going on (looks like you were debugging it), judging from that.

How do I kick the CI to get it to start again? It seems to have stopped at linting this time.

Copy link
Member

Choose a reason for hiding this comment

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

That was me? 😕 I have no recollection so I say remove it and maybe I'll get to the same conclusion again 😆

How do I kick the CI to get it to start again? It seems to have stopped at linting this time.

Either force push an amended commit, push an empty commit or close and reopen. I think GitHub is having issues as I'm seeing lost web hooks in other organisations as well.

Copy link
Member

Choose a reason for hiding this comment

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

@chrisburr can you give some references on how to debug overlinking errors that are releated to things similar to this? I'm not experienced here, but that seems to be the only thing that is fialing in my local tests.

@lgray
Copy link
Contributor Author

lgray commented Nov 8, 2022

indeed --with-cgaldir was causing problems and now it seems to build fine, but it fails when messing about with the libraries.

@@ -26,11 +26,13 @@ requirements:
- automake
- autoconf
- swig
- cgal-cpp
Copy link
Member

Choose a reason for hiding this comment

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

This belongs in host.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes just noticed that!

@chrisburr
Copy link
Member

You also need gmp in host:

  ERROR (fastjet,lib/python3.10/site-packages/fastjet/_fastjet_core/lib/libfastjet.so.0.0.0): Needed DSO lib/libgmp.so.10 found in ['gmp']

@lgray
Copy link
Contributor Author

lgray commented Nov 8, 2022

@chrisburr so it's getting all the way to the point where it's built the libraries, etc. However, there are massive complaints of overlinking in the final products. I've put the meta.yaml back into a state without the dso_whitelist and that's about the limit of what I know how to do effectively as far as debugging goes.

@lgray
Copy link
Contributor Author

lgray commented Nov 8, 2022

OK - trying that... Let's see :D

@chrisburr
Copy link
Member

You'll probably also need to add the dso whitelist stuff to at least see it pass (or see it fail with a missing shared library when testing).

Tomorrow I can run a build locally and figure out why the fastjet libs are badly linked.

@chrisburr
Copy link
Member

I suspect you're now seeing a swig 4.1+ thing, basically ClusterSequence_fastjet_banner_stream -> ClusterSequence.fastjet.banner.stream. (This also works with older swig versions.)

@conda-forge-webservices
Copy link

Hi! This is the friendly automated conda-forge-linting service.

I just wanted to let you know that I linted all conda-recipes in your PR (recipes/fastjet) and found it was in an excellent condition.

@lgray
Copy link
Contributor Author

lgray commented Mar 7, 2023

Something weird is happening with fastjet contrib. @chrisburr help appreciated. The rest seems to be building happily now, though, so we're perhaps closer than before.

@lgray
Copy link
Contributor Author

lgray commented Mar 7, 2023

Oh, I think I see... No, no idea why it's suddenly failing.

@chrisburr
Copy link
Member

It's trying to use g++ and not respecting $CXX when building fastjet-contrib.

@lgray
Copy link
Contributor Author

lgray commented Mar 7, 2023

Glorious.

@lgray
Copy link
Contributor Author

lgray commented Mar 28, 2023

@chrisburr so what's going on is that I need to supply CXX to the ./configure of fastjet-contrib but it's parsing the rest of the arguments in a completely obtuse way that I don't understand so if I specify more than CXXFLAGS or CXX individually one argument eats the other.

Help appreciated. :-)

@chrisburr
Copy link
Member

From a glance at the commits you've tried I've noticed something odd:

                    f'CXX={env.get("CXX", "g++")} CXXFLAGS=-O3 -Bstatic -Bdynamic -std=c++17',

These aren't split correctly, you're setting CXX to {env["CXX"} CXXFLAGS=-O3 -Bstatic -Bdynamic -std=c++17.

You probably need something like:

                    f'CXX={env.get("CXX", "g++")}',
                    "CXXFLAGS=-O3 -Bstatic -Bdynamic -std=c++17",

@lgray
Copy link
Contributor Author

lgray commented Apr 20, 2023

The current issue with not completing the build seems unrelated or am I reading that wrong? It looks like somehow the fastjet contrib package is missing files or they are mislocated???

lgray and others added 20 commits February 22, 2024 13:45
Co-authored-by: Matthew Feickert <[email protected]>
* fastjet-contrib's .Makefile.inc is created at ./configure time, which means
  that is the environment's CXX is accessible inside of setup.py it can be passed
  in as an argument to configure. Use this to set CXX for fastjet-contrib/.Makefile.inc
  and use a default value of g++ if no environment value is set.
* Remove g++ from build requirements.
* Add the libsiscone.so.0, libsiscone_spherical.so.0, and libfastjetplugins.so.0
  shared libraries to the missing_dso_whilelist. All exist under
  lib/python3.*/site-packages/fastjet/_fastjet_core/lib/.
* Add gmp to the 'run' requirements as fastjet does not currently package this
  shared library but it is required at runtime.
* Amend the 0001-Don-t-overwrite-external-CXXFLAGS-and-LDFLAGS.patch.
   - Fully remove the logic for donwloading CGAL as this is covered by the
     'host' requirements.
   - Move the update of the CXX environment variable till after the os.environ.copy()
     called just before the fastjet-contrib ./configure to ensure that a default
     exists.
   - Remove the '$$' from '$ORIGIN', keeping ony one '$'. This is important, else the
     LDFLAGS will not resolve properly.
Co-authored-by: Matthew Feickert <[email protected]>
* Patch fastjet-contrib/Makefile.in to use the environment variable $LDFLAGS
  during the build of the libfastjetcontribfragile shared library.
* Patch setup.py to properly set LDFLAGS and provide the environment variables
  at ./autogen.sh time and ./configure time when the fastjet-core and fastjet-contrib
  Makefiles are being generated.
* Move gmp to the 'host' requirements.
* Sort the 'build' and 'host' requirements.
* Use single quotes and '$$' to properly guard ORIGIN when LDFLAGS are
  injected into Makefiles.
  e.g. '$$ORIGIN/_fastjet_core/lib'
  Without this guard, when Make evaluates the variable $ORIGIN will get
  evaluated as RIGIN.
* As conda-forge's build procedure was trying to search through other CPython versions
  than the target's CPython in the build_artifacts it was then attempting to link
  incorreclty and causing overlinking errors. If a wheel is built first and then
  fastjet is installed from that wheel, then the resulting shared libraries
  will be linked correctly and the build can pass without errors.
Co-authored-by: Matthew Feickert <[email protected]>
Copy link
Member

@xhochy xhochy left a comment

Choose a reason for hiding this comment

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

Overall looking good. We can merge on gree CI

@matthewfeickert
Copy link
Member

matthewfeickert commented Feb 22, 2024

Overall looking good. We can merge on green CI

Fantastic! Thank you for your reviews @xhochy!

@ocefpaf ocefpaf merged commit 4c9c6dc into conda-forge:main Feb 22, 2024
5 checks passed
@lgray lgray deleted the fastjet branch February 22, 2024 21:05
@matthewfeickert
Copy link
Member

(While this is obvious to people who know, for those who might get linked here by me) The feedstock generated from this PR can be found over on https://github.com/conda-forge/fastjet-feedstock 👍

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

Successfully merging this pull request may close these issues.

9 participants