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

Freud 2.10.0 release with macos-arm64 builds. #48

Merged
merged 45 commits into from
May 26, 2022

Conversation

joaander
Copy link
Contributor

@joaander joaander commented Apr 28, 2022

Checklist

  • Used a personal fork of the feedstock to propose changes
  • Bumped the build number (if the version is unchanged)
  • Reset the build number to 0 (if the version changed)
  • 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.

This is another attempt to see if the scikit-build cross-compilation for macos-arm64 works any better.

@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.

@joaander
Copy link
Contributor Author

ppc64le builds are failing without any log.
@conda-forge-admin, please restart ci

@joaander joaander changed the title Enable macos-arm64 builds Freud 2.10.0 release with macos-arm64 builds. May 18, 2022
@joaander joaander marked this pull request as ready for review May 18, 2022 14:32
Copy link
Contributor

@bdice bdice left a comment

Choose a reason for hiding this comment

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

Thanks @joaander for your hard work on this!

# Filter CMAKE_PREFIX_PATH out of CMAKE_ARGS because scikit-build needs to set it
export SKBUILD_CONFIGURE_OPTIONS=$(echo $CMAKE_ARGS | sed -e 's/\-DCMAKE_INSTALL_PREFIX\=[^ ]* //g')

# work around a bug in conda-forge where the installed cython gets an invalid #! line: "#!$BUILD_PREFIX/bin/python"
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this a bug that needs to be filed to the conda-forge Cython feedstock, or the upstream Cython? If so, can we file that and cross-link it here?

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 find it very challenging to debug issues with prefixes in the conda-forge build as there appears to be some sort of output processor that converts /path/to/conda-env/placeholder/placeholder/... to $PREFIX in the display. For example, echo $PREFIX always outputs $PREFIX. Thus, in tests that cat cython for example, I can't tell whether there is really a literal $PREFIX in the file, or the actual absolute path.

In the case of cython, the behavior I get without the patch is consistent with a literal $PREFIX existing in the file. I do not know what stage of the conda build and/or install process puts this here, so I wouldn't know where to submit an upstream bug.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Specifically, I don't know whether this is caused in upstream cython, in the cython conda-forge feedstock, in conda's installation relocation mechanism, in the conda-forge docker build scripts, or in some combination of the above.

Copy link

Choose a reason for hiding this comment

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

Yeah this process is difficult to debug. I don't think the problem can be in upstream Cython, and I think the relocation is the most likely culprit, but I don't know how to verify that it's not any other part of the conda process without spending a lot more time searching. The feedstock looks correct to me, and when I download builds the directory is set correctly in the console entry point script, but that just means that there are lots of situations where they aren't currently running into this problem. If we get a chance later we can try to follow up, but I'm fine with getting this merged to unblock freud wheels.

@joaander
Copy link
Contributor Author

For reference, the pip bug is discussed in pypa/pip#11116 and conda-forge/mdanalysis-feedstock#42. Once pip releases a fix we can update the version specification accordingly.

Vyas, I had to remove the tests because they are not included in the source tarball provided on pypi and there is no alternate source tarball. When such tests are added, we can uncomment the pytest line again. But for the moment we will be publishing untested binaries.

Copy link
Contributor

@tommy-waltmann tommy-waltmann left a comment

Choose a reason for hiding this comment

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

Looks good!

Copy link

@b-butler b-butler left a comment

Choose a reason for hiding this comment

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

This looks good to me, though this is outside my general software knowledge.

Copy link

@vyasr vyasr left a comment

Choose a reason for hiding this comment

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

I'm good with moving forward here. We can do cleanup and add tests later if we're able to resolve the underlying Cython issues and get tests into our tarballs.

@@ -0,0 +1,28 @@
export VERBOSE=1
Copy link

Choose a reason for hiding this comment

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

Do we want compilation to always be verbose?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It adds a moderate amount to the log size, but provides useful debugging information if the builds ever break in the future. That makes one less step to debugging a broken build.

@tommy-waltmann tommy-waltmann merged commit 79ecb8d into conda-forge:main May 26, 2022
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.

None yet

6 participants