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

Package openfoam-org and add openfoam NixOS module #329527

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

gusgibbon
Copy link

@gusgibbon gusgibbon commented Jul 23, 2024

Description of changes

Added a new package: openfoam-org
openfoam.org fork of OpenFOAM
Added a new NixOS module: programs.openfoam
Creates a ofoam shell alias for loading the OpenFOAM environment

Closes #131486.
Closes #346976.

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 24.11 Release Notes (or backporting 23.11 and 24.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

@github-actions github-actions bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` 8.has: maintainer-list (update) This PR changes `maintainers/maintainer-list.nix` labels Jul 23, 2024
@NixOSInfra NixOSInfra added the 12. first-time contribution This PR is the author's first one; please be gentle! label Jul 23, 2024
@gusgibbon gusgibbon force-pushed the master branch 3 times, most recently from 6d5e730 to effd0a0 Compare July 25, 2024 20:58
@ofborg ofborg bot added 8.has: package (new) This PR adds a new package 11.by: package-maintainer This PR was created by the maintainer of the package it changes 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 labels Jul 25, 2024
pkgs/by-name/op/openfoam-org/package.nix Outdated Show resolved Hide resolved
pkgs/by-name/op/openfoam-org/package.nix Outdated Show resolved Hide resolved
pkgs/by-name/op/openfoam-org/package.nix Show resolved Hide resolved
pkgs/by-name/op/openfoam-org/package.nix Outdated Show resolved Hide resolved
@gusgibbon gusgibbon force-pushed the master branch 3 times, most recently from ae2f88a to a2547e1 Compare October 24, 2024 14:32
];
buildInputs = [
fftw
mpi
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
mpi

Builds without. I think this is covered by mpi.dev below, no?

Copy link
Author

Choose a reason for hiding this comment

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

I believe you are correct, I probably just left that in there when packaging this.

Copy link
Contributor

Choose a reason for hiding this comment

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

mpi in builtInputs is automatically taken with getDev already so you don't need to specify .dev, and it's better not to specify so that we don't enforce that the output should necessarily be split out

@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/package-openfoam-for-nixpkgs/47096/5

Copy link
Contributor

@SomeoneSerge SomeoneSerge Oct 24, 2024

Choose a reason for hiding this comment

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

I've just come across this and #329213 (following the discourse thread). I didn't grasp the full context yet but I can spend a few hours this or next week. Which one would it be more prudent to review? (CC @razielgn)

Copy link
Author

Choose a reason for hiding this comment

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

Both of these PRs are for different forks of OpenFOAM, each with some fairly significant differences between them. I'm not sure which one is more popular.

@razielgn, we should probably find a consistent way to make the binaries accessible across forks before these PRs are merged. I'm using a shell alias to load the OpenFOAM environment similar to how arch does it, as the OpenFOAM bashrc is quite large, but I would be open to other methods.

Copy link
Contributor

@razielgn razielgn Oct 25, 2024

Choose a reason for hiding this comment

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

Yes, unfortunately there are two forks of OpenFOAM.

@gusgibbon You talked about having the two collections of binaries available. Do you mean in the same $PATH? I wouldn't expect it to be a common use case. I mean, you either use one or the other, right?

In my derivation, to get around the fact that OpenFOAM's binaries require that obscure env setup via etc/bashrc (if I recall the name correctly), I'm wrapping each one in order for that initialisation to take place transparently. It might not be the best solution, for sure there's a tiny amount of time wasted on every invocation, but in my opinion it provides a "least surprise" effect.

That said, @SomeoneSerge, I just yesterday found out that a dependency's derivation (scotch) has recently been split out into $out and $dev, my derivation still compiles but fails to see it and use it, resulting in a crash at runtime on a subset of binaries. This is due to the rigidness and frankly awkward build system that OpenFOAM implements. I still need to figure out how to fix this.

Copy link
Author

Choose a reason for hiding this comment

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

I apologize, I worded that unclearly.

What I was getting at is specifically that bashrc environment setup you mentioned, which I believe is the same for both forks. The NixOS module in this PR creates a "ofoam" shell alias to load that bashrc, as opposed to your wrapping technique which places binaries directly in $PATH and loads the bashrc upon execution. I suggest that we decide on one of these two methods for both packages, as currently there would be a NixOS module that only works with the org fork, which I imagine would be confusing to users.

My argument for the shell alias is that it provides much more flexibility if multiple versions are installed, which I imagine is a somewhat common use case given the prevalence of custom solvers/boundary conditions/etc. Additionally it allows for two forks to be used simultaneously. Personally, I use the com fork occasionally for meshing, however as you said this is probably not a typical use case. The shell alias is less intuitive than your approach, however I think the pros outweigh the cons, and considering the OpenFOAM install instructions for other distros guide you through using the bashrc environment setup, I don't think many users would get lost with the shell alias. Regardless, I'm not very opinionated on this, and would be willing to help adjust the packages in either circumstance.

Copy link
Contributor

Choose a reason for hiding this comment

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

My opinion is still uninformed, but generally speaking the wrapper approach sounds more flexible in terms of multi-tenancy and less invasive?

Copy link
Contributor

Choose a reason for hiding this comment

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

An assumption of mine: if people would decide to use OpenFOAM through nixpkgs instead of going for the official debs, it means they are supposed to be at least somewhat familiar with the nixpkgs way of things.

Now a few thoughts:

  • Supporting simultaneous multiple versions probably could solved at a higher level, meaning through nix you can mix and match by setting up $PATHs to accomodate one own's personal needs.
  • It is true that the "official" way of using OpenFOAM is to first go through source etc/bashrc, then invoke bash functions/binaries. This contrasts with my initial assumption above, meaning that through documentation we can show how to use it, so that it better integrates in the nixpkgs ecosystem. This covers the development of OpenFOAM extensions as well, as I documented in my PR.
  • Elaborating on the wrapper approach, as @SomeoneSerge wrote, it's more flexible because each invocation of a binary is self-contained and not invasive. It means that the case of using one binary from version X and one binary from version Y is totally fine and wouldn't create issues.

Copy link
Author

Choose a reason for hiding this comment

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

Now that you mention it, I think you are right in that using multiple versions should just be done through nix. I'll rewrite to use the wrapping method when I have time.

@gusgibbon gusgibbon marked this pull request as draft October 28, 2024 12:14
@wegank wegank added the 2.status: merge conflict This PR has merge conflicts with the target branch label Nov 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2.status: merge conflict This PR has merge conflicts with the target branch 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: changelog 8.has: documentation This PR adds or changes documentation 8.has: maintainer-list (update) This PR changes `maintainers/maintainer-list.nix` 8.has: module (update) This PR changes an existing module in `nixos/` 8.has: package (new) This PR adds a new package 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 11.by: package-maintainer This PR was created by the maintainer of the package it changes 12. first-time contribution This PR is the author's first one; please be gentle!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Package request: openfoam Package OpenFOAM for FreeCAD's Cfd workbench
7 participants