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

Fix setuptools on conda-forge feedstock #179

Merged
merged 2 commits into from
Jul 18, 2024

Conversation

altheaden
Copy link
Collaborator

@altheaden altheaden commented Jul 17, 2024

A previously added setuptools option, setuptools-scm, was set in #175 during the transition from setup.cfg to pyproject.toml, and this option ended up not working on the conda-forge feedstock. This PR removes that option and replaces it again with the MANIFEST.in file.

Checklist

  • Testing comment in the PR documents testing used to verify the changes

@altheaden
Copy link
Collaborator Author

@xylar I am having a hard time testing that this setup works. For some reason, if I create the environment without the MANIFEST.in file, I still have all the necessary files (like the .cfg and .xml files) loaded into the package. Thus, I am not sure where the MANIFEST.in file is used or what its purpose is. This makes me think that I might have something switched on that will give us problems later. Here is my testing script:

#!/bin/bash

conda activate base
yes | conda env remove -n mache_dev
conda config --add channels conda-forge 
conda config --set channel_priority strict 
conda create -y -n mache_dev --file spec-file.txt 
conda activate mache_dev 
python -m pip install .

and then I am checking for the .cfg files in /Users/althea/miniconda3/envs/mache_dev/lib/python3.12/site-packages/mache/machines, and they are always there.

@xylar
Copy link
Collaborator

xylar commented Jul 18, 2024

Let's change "Azure Pipelines" to "the conda-forge feedstock". I know it's a subtlety but the issue is that the build isn't working when the code doesn't come from GitHub, not which CI it's building on.

@xylar
Copy link
Collaborator

xylar commented Jul 18, 2024

Regarding testing, one thing I discovered in my own testing is that we need to remove the build and mache.egg-info directories if they are left over from a previous build. Otherwise, pip install or compass build can use info in those directories to find out what files it's supposed to include. Maybe that's what's happening on your Mac.

To be on the safe side, I would also try testing on Perlmutter rather than your laptop. Perlmutter runs on something like Linux, so that would eliminate the possibility that MacOS is responsible for the different behavior.

Also it would be good to test with downloaded source code rather than a git repository. (This is what didn't work with setuptools-scm.) You can download the source for a specific commit hash (in this case altheaden@15459c2) like this:

wget https://github.com/altheaden/mache/archive/15459c25bd0f7acaabb924edc9cac49460f4982c.tar.gz

Then, you can untar the file and to a test install:

tar xvf 15459c25bd0f7acaabb924edc9cac49460f4982c.tar.gz
cd mache-15459c25bd0f7acaabb924edc9cac49460f4982c

pm_conda
conda create -y -n mache_dev --file spec-file.txt 
conda activate mache_dev 
python -m pip install .
ls $HOME/miniforge3/envs/mache_dev/lib/python3.12/site-packages/mache/machines

Then, you can try:

rm MANIFEST.in
# these files seem to remember the info from the previous build
rm -rf build mache.egg-info
# this will also remove the old mache_dev environment
conda create -y -n mache_dev --file spec-file.txt 
conda activate mache_dev 
python -m pip install .
ls $HOME/miniforge3/envs/mache_dev/lib/python3.12/site-packages/mache/machines

I tried this on Chrysalis and the first one works successfully (the config files are there) but the second one doesn't.

You can also try using conda build with a fresh checkout:

wget https://github.com/altheaden/mache/archive/15459c25bd0f7acaabb924edc9cac49460f4982c.tar.gz
tar xvf 15459c25bd0f7acaabb924edc9cac49460f4982c.tar.gz
cd mache-15459c25bd0f7acaabb924edc9cac49460f4982c

pm_conda
conda build conda

rm MANIFEST.in
conda build conda

Again, I'm seeing that the first will succeed and the second fails.

Maybe your testing was successful even without the MANIFEST.in because had the build and mache.egg-info directories left over from a previous build. Or maybe the behavior of setuptools is different on the Mac. That would be frustrating but not unheard of.

Copy link
Collaborator

@xylar xylar left a comment

Choose a reason for hiding this comment

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

This worked for me on Chrysalis even outside of a git repository, whereas I am seeing failures without the MANIFEST.in.

@xylar xylar added the bug Something isn't working label Jul 18, 2024
@altheaden
Copy link
Collaborator Author

@xylar Perfect, that sounds like exactly what I needed. I'll stick to testing on PM by default. Do you want me to do any additional testing for this PR before we merge? It sounds like you only tested on Chrysalis so I'll go ahead and set things up to test on PM myself.

@altheaden altheaden changed the title Fix setuptools on Azure Pipelines Fix setuptools on conda-forge-feedstock Jul 18, 2024
@altheaden altheaden changed the title Fix setuptools on conda-forge-feedstock Fix setuptools on conda-forge feedstock Jul 18, 2024
@altheaden
Copy link
Collaborator Author

@xylar just tested this branch on perlmutter and it worked exactly as expected. Without MANIFEST.in, the non-python files were missing, and with it they were present. To be safe I deleted the build and mache.egg-info directories between the tests.

@xylar
Copy link
Collaborator

xylar commented Jul 18, 2024

Great, merge when you're ready.

@altheaden altheaden merged commit 82466ae into E3SM-Project:main Jul 18, 2024
6 checks passed
@altheaden altheaden deleted the fix-setuptools-CI branch July 18, 2024 17:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants