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

Update libcantera-devel #35

Merged
merged 5 commits into from
Aug 27, 2022
Merged

Update libcantera-devel #35

merged 5 commits into from
Aug 27, 2022

Conversation

ischoegl
Copy link
Member

@ischoegl ischoegl commented Aug 11, 2022

This PR includes some minor updates:

  • Include Fortran modules in libcantera-devel
  • Create "clean" sample folders for cxx and f90
  • Install man pages in cantera
  • Ensure that libboost is a dependency for libcantera-devel
  • Fix extra level in matlab folder structure
  • Add SCons option package_build = True

Closes #37.

@bryanwweber
Copy link
Member

  • Ensure that libboost is a dependency for libcantera-devel

This shouldn't be needed, as we only rely on the header portions of the library? The concern I have is that this will then bring in libboost for the cantera package too (because cantera depends on libcantera-devel).

@ischoegl
Copy link
Member Author

ischoegl commented Aug 11, 2022

  • Ensure that libboost is a dependency for libcantera-devel

This shouldn't be needed, as we only rely on the header portions of the library? The concern I have is that this will then bring in libboost for the cantera package too (because cantera depends on libcantera-devel).

Since libcantera-devel depends on libcantera, there shouldn’t be a concern? I can confirm once the packages are done building. (This is still WIP after all 😆). I may have to make sure it’s headers only, but libcantera-devel does indeed need them and they currently aren’t installed.

@bryanwweber
Copy link
Member

Ah, if cantera depends on libcantera, then libcantera-devel is separate. I don't think I realized we'd made that split. All should be fine then!

@ischoegl ischoegl marked this pull request as ready for review August 11, 2022 17:38
@ischoegl
Copy link
Member Author

ischoegl commented Aug 11, 2022

@bryanwweber ... it is ready now. I also just confirmed that

conda install -c file:///opt/conda/envs/conda-bld/conda-bld/ libcantera-devel

and

conda install -c file:///opt/conda/envs/conda-bld/conda-bld/ cantera

have the expected dependencies.

PS: since the latest GH Action, the only change was to remove a ignore_run_exports_from from the libcantera-devel recipe (which negates the dependency). A local build passes, so I think it's good to go.

Copy link
Member

@bryanwweber bryanwweber left a comment

Choose a reason for hiding this comment

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

Two small things ☺️ I wish I had more time to be productive with Cantera!

cantera/build_devel.bat Outdated Show resolved Hide resolved
cantera/build_py.bat Outdated Show resolved Hide resolved
@ischoegl
Copy link
Member Author

@bryanwweber ... thanks! Things are taken care of. I triggered a GH Action to build the packages just to make sure.

@ischoegl
Copy link
Member Author

ischoegl commented Aug 12, 2022

@bryanwweber ... I documented the issues with Makefile's in #37 and in comments, and verified that Fortran modules are packaged properly. So I think this is good to go ...

Copy link
Member

@bryanwweber bryanwweber left a comment

Choose a reason for hiding this comment

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

Another couple small clarifications here 😊

cantera/build_devel.bat Outdated Show resolved Hide resolved
cantera/build_devel.bat Outdated Show resolved Hide resolved
cantera/meta.yaml Outdated Show resolved Hide resolved
@ischoegl
Copy link
Member Author

ischoegl commented Aug 12, 2022

@bryanwweber ... fixed things per comments. I also caught one extra-level introduced by copying of some folders of the cantera-matlab package on *nix, which is also fixed.

@ischoegl ischoegl marked this pull request as draft August 13, 2022 00:20
@ischoegl
Copy link
Member Author

@bryanwweber ... I'd suggest to table the discussion on whether or not to package 'broken' Makefile's. The issue is now documented - see #37 - and without it, this PR still introduces some features that are worthwhile (see top).

@ischoegl ischoegl marked this pull request as ready for review August 13, 2022 03:08
@ischoegl
Copy link
Member Author

ischoegl commented Aug 17, 2022

@bryanwweber …. As things stands, this PR is independent of #37 and Cantera/cantera#1366. As such, this is converged on my side.

Now that Cantera/cantera#1366 is merged, the missing SCons option can be added here.

@ischoegl
Copy link
Member Author

ischoegl commented Aug 26, 2022

Using artifacts from a CI run on GitHub Actions, I have confirmed that #37 is resolved on Linux. Tests on windows revealed issues beyond #37, although some of the build routes are broken even for a regular install (see Cantera/cantera#1375 etc.) and thus need to be taken care of upstream first.

I haven't had a chance to test on macOS yet, but don't expect any surprises as the recipe is largely identical to that of Linux. Edit: On macOS, CMake works for C++ examples, although make and SCons appear to miss some linker flags (it does work for a local build, although there are some issues with Fortran samples even there, see Cantera/cantera#1378).

And of course, SCons/scons#3664 is again popping up on Windows when trying to use SCons to compile samples ... (although it does appear like a resolution of that issue may be on the horizon).

Edit: Summarizing, this PR resolves issues on Linux; I'd suggest to address remaining issues with macOS and Windows in follow-up PR's

@bryanwweber bryanwweber merged commit 18f740e into main Aug 27, 2022
@ischoegl ischoegl deleted the update-libcantera-devel branch August 27, 2022 16:08
@ischoegl ischoegl mentioned this pull request Aug 27, 2022
5 tasks
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.

conda-build mangles libcantera-devel build system files
2 participants