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 OpenCascade Libraries to FromCabal.Name and Fix opencascade-hs in FromCabal.Postprocess #630

Merged
merged 1 commit into from
Nov 13, 2024

Conversation

joe-warren
Copy link
Contributor

@joe-warren joe-warren commented Nov 4, 2024

This PR includes two changes:

FromCabal.Name: add missing opencascade-occt library names

The OpenCascade OCCT library is distributed as multiple "toolkits", which in practice are separate link libraries.
In nix-packages, these are all bundled under opencascade-occt.
Adding these to FromCabal.Name lets us infer the correct nix package name from the extra-libraries name.

FromCabal.PostProcess: add opencascade-hs extra-include-dirs

Header files in the OpenCascade library are sorted under include/opencascade.
This needs to be explicitly passed to the extra-include-dirs configure flag of opencascade-hs for this package to build


Of the two above changes, I'm less confident that the second one is the correct thing to do, and I'm very open to other suggestions regarding it.

The OpenCascade OCCT library is distributed as multiple "toolkits", which in practice are separate link libraries.
In nix-packages, these are all bundled under opencascade-occt.
@joe-warren
Copy link
Contributor Author

Updating configuration-common in nixpackages

A friend of mine came up with this patch to nix packages, which also works to fix the build.

This flake references that branch, and demonstrates that this fixes the build.

It’s not particularly clear to me which of these two solutions is best?

I think there might be a case for updating libNixName, but keeping the extra-include-dir in configuration-common.
libNixName is a mapping from library name to nix package, and making this more complete would seem to be a good thing, whereas by comparison, the --extra-include-dir flag is a hack to work around the limitations in the way the underlying opencascade library have set up their imports.
On the down side, this involves making changes in two separate places.

@sternenseemann
Copy link
Member

I'm not convinced the --extra-include-dirs change is right either way. It is quite common for header files to be grouped in subdirectories of include as they are still reachable from the normal compiler search path. The correct and portable way to access opencascade headers would be to use #include <opencascade/my-header.h>, it seems. It's probably simplest to just change opencascade-hs. What made you access the headers in the way you do in that project?

@joe-warren
Copy link
Contributor Author

I'm not convinced the --extra-include-dirs change is right either way. It is quite common for header files to be grouped in subdirectories of include as they are still reachable from the normal compiler search path. The correct and portable way to access opencascade headers would be to use #include <opencascade/my-header.h>, it seems. It's probably simplest to just change opencascade-hs. What made you access the headers in the way you do in that project?

I agree in general, this would be better, however, if you take a look at the header files in the underlying opencascade library the contents of the header files themselves reference each other without using an opencascade/ prefix.

So unless I'm missing something, the opencascade directory needs to be on the include path?

@sternenseemann
Copy link
Member

Okay, OpenCASCADEConfig.cmake also picks out the include dir, how weird. I'd say let's put the extra override into nixpkgs for now and add the mappings to cabal2nix.

@joe-warren joe-warren force-pushed the fix-opencascade-hs-build branch from 8bb55c5 to 508bb00 Compare November 11, 2024 00:26
@joe-warren
Copy link
Contributor Author

Okay, OpenCASCADEConfig.cmake also picks out the include dir, how weird. I'd say let's put the extra override into nixpkgs for now and add the mappings to cabal2nix.

I've updated the PR to remove the override, but keep the libNixName mappings

@joe-warren
Copy link
Contributor Author

Okay, OpenCASCADEConfig.cmake also picks out the include dir, how weird. I'd say let's put the extra override into nixpkgs for now and add the mappings to cabal2nix.

I've updated the PR to remove the override, but keep the libNixName mappings

To clarify, I think this can now be merged as is

@maralorn maralorn merged commit b5e2ba8 into NixOS:master Nov 13, 2024
5 checks passed
joe-warren added a commit to joe-warren/nixpkgs that referenced this pull request Nov 15, 2024
As of [this PR](NixOS/cabal2nix#630)
opencascade-hs does not require the dependency on opencascade-occt to be manually specified.

It does however still require the include path to be defined.
joe-warren added a commit to joe-warren/nixpkgs that referenced this pull request Nov 15, 2024
As of [this PR](NixOS/cabal2nix#630)
opencascade-hs does not require the dependency on opencascade-occt to be manually specified.

It does however still require the include path to be defined.
@joe-warren
Copy link
Contributor Author

@maralorn Do I need to do anything with regards to getting this change included in nixpkgs and rerunning hackage2nix on opencascade-hs?

Thanks again for your help

@maralorn
Copy link
Member

Good point. You can open a PR against the haskell-updates branch in nixpkgs where you run maintainers/scripts/haskell/update-cabal2nix-unstable.sh.

@joe-warren
Copy link
Contributor Author

Good point. You can open a PR against the haskell-updates branch in nixpkgs where you run maintainers/scripts/haskell/update-cabal2nix-unstable.sh.

This fails to build on my machine, so I might now be dependent on someone else running that

joe-warren added a commit to joe-warren/nixpkgs that referenced this pull request Nov 29, 2024
As of [this PR](NixOS/cabal2nix#630)
opencascade-hs does not require the dependency on opencascade-occt to be manually specified.

It does however still require the include path to be defined.
joe-warren added a commit to joe-warren/nixpkgs that referenced this pull request Dec 8, 2024
As of [this PR](NixOS/cabal2nix#630)
opencascade-hs does not require the dependency on opencascade-occt to be manually specified.

It does however still require the include path to be defined.
joe-warren added a commit to joe-warren/nixpkgs that referenced this pull request Dec 8, 2024
As of [this PR](NixOS/cabal2nix#630)
opencascade-hs does not require the dependency on opencascade-occt to be manually specified.

It does however still require the include path to be defined.
sternenseemann added a commit to NixOS/nixpkgs that referenced this pull request Dec 27, 2024
As of [this PR](NixOS/cabal2nix#630)
opencascade-hs does not require the dependency on opencascade-occt to be
manually specified. It does however still require the include path to be
defined.

Co-authored-by: sternenseemann <[email protected]>
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.

3 participants