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

Adding Medcoupling #24378

Merged
merged 46 commits into from
Mar 15, 2024
Merged

Adding Medcoupling #24378

merged 46 commits into from
Mar 15, 2024

Conversation

Krande
Copy link
Contributor

@Krande Krande commented Oct 30, 2023

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).
  • 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-webservices
Copy link

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

I wanted to let you know that I linted all conda-recipes in your PR (recipes/medcoupling) and found some lint.

Here's what I've got...

For recipes/medcoupling:

  • There are too few lines. There should be one empty line at the end of the file.
  • license_file entry is missing, but is required.

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

@Krande
Copy link
Contributor Author

Krande commented Oct 30, 2023

@conda-forge-admin please rerender

@Krande
Copy link
Contributor Author

Krande commented Oct 30, 2023

@conda-forge/help-python-c

@Krande
Copy link
Contributor Author

Krande commented Oct 30, 2023

@hmaarrfk I really appreciate the review! Also I believe your suggestions and comments should be implemented/answered by now. I tested this locally and it worked, so hopefully the CI agrees.

@Krande Krande closed this Mar 12, 2024
@Krande Krande reopened this Mar 12, 2024
@Krande Krande closed this Mar 12, 2024
@Krande Krande reopened this Mar 12, 2024
@Krande
Copy link
Contributor Author

Krande commented Mar 12, 2024

Hey @hmaarrfk, I ended up using 32 bit integers for the windows variant for both libmed (build 11) and medcoupling. Now all checks are passing for windows and Linux (we can sort osx support at a later stage after the PR is merged).

Is there anything else you wanted me to do before merging the PR?

@hmaarrfk
Copy link
Contributor

I noticed @jschueller had opened #23258

maybe you want to comment?

@hmaarrfk
Copy link
Contributor

@Krande I must apologize and admit that seeing typedefs change based on compilation options triggered horror stories I exprienced years ago when building some of my first software executables.

To be honest, conda(-forge) does not have a good to track these. We can understand "version numbers" quite well, but not "different compilation options".

So i would have two requests:

  1. Are you sure you are OK with your windows compilations options?
  2. Can you make any packages that were compiled with incompatible compilations as broken following https://conda-forge.org/docs/maintainer/updating_pkgs/#removing-broken-packages

@Krande Krande mentioned this pull request Mar 14, 2024
10 tasks
@Krande
Copy link
Contributor Author

Krande commented Mar 14, 2024

@Krande I must apologize and admit that seeing typedefs change based on compilation options triggered horror stories I exprienced years ago when building some of my first software executables.

To be honest, conda(-forge) does not have a good to track these. We can understand "version numbers" quite well, but not "different compilation options".

I understand your concerns. And I will make it a priority of keeping the conda-forge libmed and medcoupling packaging as simple as possible (ie. no extra variants). If there are any additional variants needed, I can upload those variants to a dedicated conda channel (either my "krande" channel or create a "code_aster" channel if needed).

So i would have two requests:

  1. Are you sure you are OK with your windows compilations options?
  2. Can you make any packages that were compiled with incompatible compilations as broken following https://conda-forge.org/docs/maintainer/updating_pkgs/#removing-broken-packages
  1. To strengthen my confidence in the current compilation options on Windows, I'll make an effort to run all the unittests on windows I can find for both med and medcoupling to ensure that the code itself runs as expected on windows. If all tests are passing (skipping any "big int" tests if there are any) then I will assume it is OK. I guess all use-cases involving simulations using big integers should be applicable only for linux versions. I'll let you know once I've completed the testing.

  2. Yes, definitively. As of now, I do not know of any downstream packages that are relying on the libmed package. So what I can do once medcoupling is merged I can follow the steps in https://conda-forge.org/docs/maintainer/updating_pkgs/#removing-broken-packages and remove all the old libmed builds. If you want I can tag you in that PR so that you can comment on it.

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

@hmaarrfk
Copy link
Contributor

And I will make it a priority of keeping the conda-forge libmed and medcoupling packaging as simple as possible (ie. no extra variants).

I think it is entirely ok to have variants. Just they are hard to maintain, and sometimes lead to more mistakes. Its hard to get them right, so its really simpler to not have them!

Thanks for your perseverance here!

@Krande
Copy link
Contributor Author

Krande commented Mar 14, 2024

Hello again, @hmaarrfk!

I have managed to test medcoupling on windows using a few different approaches and the results are very promising. So promising in fact, that I feel OK with the windows compilation options as they are now.

I should mention that I have only tested the swig wrapped python library (as the cpp unittests rely on CppUnit which as far as I can tell is not available on windows).

So after tried several of the unittest files inside the medcoupling src/ directories. It seems like several of the tests are outdated or otherwise not meant to be tested on windows. Nonetheless, with a few adjustments (removing linux only imports and removing some outdated import statements) I managed to get the majority of the tests passing.

I also checked with a downstream library medpro (https://github.com/ldallolio/medpro) which is a pythonic wrapper of medcoupling where I managed to get all tests passing.

@hmaarrfk If you have no objections to the simplified build and install commands as suggested by @jschueller I will make a last commit where these changes are added?

@hmaarrfk
Copy link
Contributor

it seems you thought through the implication quite a bit.

Feel free to simplify as suggested, we can merge when it passes again.

@Krande
Copy link
Contributor Author

Krande commented Mar 15, 2024

It looks like all tests are passing after I made the last changes. @hmaarrfk merge whenever you are ready :)

@hmaarrfk hmaarrfk merged commit 47b3f38 into conda-forge:main Mar 15, 2024
5 checks passed
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.

4 participants