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

[DO NOT MERGE] Testing a switch to Cmake #52

Open
wants to merge 22 commits into
base: main
Choose a base branch
from

Conversation

gonuke
Copy link
Contributor

@gonuke gonuke commented Aug 23, 2021

Checklist

  • Used a personal fork of the feedstock to propose changes
  • Bumped the build number (if the version is unchanged)
  • Re-rendered with the latest conda-smithy (Use the phrase @conda-forge-admin, please rerender in a comment in this PR for automated rerendering)
  • Ensured the license file is being packaged.

Attempting to switch to CMake build instead of autotools.

@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 (recipe) and found it was in an excellent condition.

@gonuke
Copy link
Contributor Author

gonuke commented Aug 23, 2021

@conda-forge-admin, please rerender

@gonuke gonuke marked this pull request as ready for review August 23, 2021 00:03
@gonuke gonuke changed the title Cmake [DO NOT MERGE] Testing a switch to Cmake Aug 23, 2021
@gonuke gonuke closed this Aug 23, 2021
@gonuke gonuke reopened this Aug 23, 2021
@xylar
Copy link
Contributor

xylar commented Nov 13, 2021

@gonuke, did you want to switch to cmake before we merge #55 ?

@gonuke
Copy link
Contributor Author

gonuke commented Nov 13, 2021

@gonuke, did you want to switch to cmake before we merge #55 ?

Thanks for checking in. I lost track of what was failing here, so I've relaunched the tests to see if I can take another shot at this

@xylar
Copy link
Contributor

xylar commented Nov 13, 2021

#55 got merged so you should probably rebase and test on that version instead.

@gonuke
Copy link
Contributor Author

gonuke commented Nov 13, 2021

Yeah - just did that

@vijaysm
Copy link
Contributor

vijaysm commented Nov 13, 2021

Quick question @gonuke. Is this to get conda working on Windows?

@gonuke
Copy link
Contributor Author

gonuke commented Nov 13, 2021

That's definitely a primary motivation

@gonuke
Copy link
Contributor Author

gonuke commented Nov 14, 2021

@conda-forge-admin, please rerender

mkdir bld
cd bld
cmake .. -DCMAKE_INSTALL_PREFIX=${PREFIX} \
-DENABLE_HDF5=ON \
Copy link
Contributor

Choose a reason for hiding this comment

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

It may be best to have the CMake workflow script side by side rather than replacing the workflow that has been verified for the entire build matrix. Especially if the main motivation is to get the build working on Windows first.

Copy link
Contributor

@xylar xylar Nov 14, 2021

Choose a reason for hiding this comment

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

I would be more inclined to switch over to use only cmake if possible.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fair enough. I'm going to continue testing with the CMake alone, however.

(Our full software stack prefers to build with CMake on all platforms.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would be more inclined to switch over to use only cmake if possible.

That would also be my preference, but I don't think that CMake gets "first class" support with limited resources available on the team. I've tried to keep it up-to-date for our limited use case, but it relies on a lot of (at-times-fragile) customization of package finding.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah thanks to Paul for keeping us focused on the CMake workflow even when we have been lagging support. I think it is in a decent enough state to replace autotools. I have my qualms about both workflows for configuration, but we will probably drop support for one of them soon (probably autotools). @gonuke I'll initiate a conversation in the mailing list this week.

The reason why I suggested having both workflows side by side is so that you can still get CMake working on Windows while keeping the rest of the platforms reliant on autotools for now since it's tested and verified already. And then we can make the transition along with MOAB when we deprecate autotools in the next major release.

@iulian787 Thoughts ?

Copy link
Contributor

Choose a reason for hiding this comment

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

@vijaysm and @gonuke, thank you both for clarifying.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Building w/ LAPACK gets complicated on Windows, and therefore it's an obstacle for including tempest

Choose a reason for hiding this comment

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

Would it be acceptable to include tempest and continue the move to cmake but just not build for windows (yet)? This would appear to still be a step on a good direction to me as we would still get cmake 🎉 .

Then the complicated LAPACK and windows building can be addressed at another time in another PR

@gonuke
Copy link
Contributor Author

gonuke commented Nov 14, 2021

@conda-forge-admin, please rerender

@gonuke
Copy link
Contributor Author

gonuke commented Nov 15, 2021

@conda-forge-admin, please rerender

@gonuke gonuke closed this Nov 15, 2021
@gonuke gonuke reopened this Nov 15, 2021
@gonuke
Copy link
Contributor Author

gonuke commented Nov 15, 2021

@conda-forge-admin, please rerender

@gonuke
Copy link
Contributor Author

gonuke commented Nov 15, 2021

@conda-forge-admin, please rerender

@gonuke
Copy link
Contributor Author

gonuke commented Nov 15, 2021

@conda-forge-admin, please rerender

@shimwell
Copy link

I see this PR has a few conflicts that I can't resolve as write access to the branch is needed. I was interested in seeing if the ever changing eco system has made the cmake build more possible since this PR was started. So I made an updated version of this PR over on #92 .

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.

5 participants