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

convert recipe to noarch #98

Merged
merged 26 commits into from
Jul 13, 2023
Merged

convert recipe to noarch #98

merged 26 commits into from
Jul 13, 2023

Conversation

ngam
Copy link
Contributor

@ngam ngam commented Jul 5, 2023

Closes #96

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.

@conda-forge-webservices
Copy link
Contributor

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.

@ngam
Copy link
Contributor Author

ngam commented Jul 5, 2023

@conda-forge-admin, please rerender

conda-forge-webservices[bot] and others added 2 commits July 5, 2023 00:30
@github-actions
Copy link
Contributor

github-actions bot commented Jul 5, 2023

Hi! This is the friendly automated conda-forge-webservice.

I tried to rerender for you, but it looks like there was nothing to do.

This message was generated by GitHub actions workflow run https://github.com/conda-forge/parcels-feedstock/actions/runs/5459351470.

@weiji14
Copy link
Member

weiji14 commented Jul 5, 2023

Could we just make this package noarch? I tried running grayskull pypi parcels and it gave a noarch package by default, though not sure if it's ok.

@ngam
Copy link
Contributor Author

ngam commented Jul 5, 2023

Could we just make this package noarch? I tried running grayskull pypi parcels and it gave a noarch package by default, though not sure if it's ok.

How is it on pypi? That’s ultimately the measure.

@ngam
Copy link
Contributor Author

ngam commented Jul 5, 2023

Yep, this is a damn noarch lol

@conda-forge-webservices
Copy link
Contributor

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 (recipe) and found some lint.

Here's what I've got...

For recipe:

  • noarch packages can't have selectors. If the selectors are necessary, please remove noarch: python.

@conda-forge-webservices
Copy link
Contributor

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.

recipe/meta.yaml Outdated Show resolved Hide resolved
@ngam
Copy link
Contributor Author

ngam commented Jul 5, 2023

I don't the noarch will work; they do have some c/cpp code going on: https://github.com/OceanParcels/parcels/tree/master/parcels/include

ngam and others added 2 commits July 4, 2023 23:16
Co-authored-by: Wei Ji <23487320+weiji14@users.noreply.github.com>
recipe/meta.yaml Outdated Show resolved Hide resolved
@weiji14
Copy link
Member

weiji14 commented Jul 5, 2023

I don't the noarch will work; they do have some c/cpp code going on: https://github.com/OceanParcels/parcels/tree/master/parcels/include

Hmm yeah, I noticed that too, but it was worth a shot 😆 This recipe is pulling the tar.gz from GitHub, so we should be able to run the unit tests to check.

@ngam
Copy link
Contributor Author

ngam commented Jul 5, 2023

Yeah, I actually think the code isn't getting compiled; it is just there as a helpers. So essentially, that's why it's requesting the compilers as a runtime dependency. I will re-add the compilers and we should definitely run more tests

@ngam ngam changed the title build for aarch64 and ppc64le on linux64 Convert recipe to noarch Jul 5, 2023
@ngam ngam changed the title Convert recipe to noarch convert recipe to noarch Jul 5, 2023
@ngam
Copy link
Contributor Author

ngam commented Jul 5, 2023

@ocefpaf we could do without outputs. The current implementation should work fine, the osx failure is due to CI issues and the win issue is that I should port in a PR from upstream to get rid of an unused entry point (OceanParcels/parcels#1379)

@ngam ngam closed this Jul 6, 2023
@ngam ngam reopened this Jul 6, 2023
Copy link

@varlackc varlackc left a comment

Choose a reason for hiding this comment

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

There seem to be some redirects in the URLs in the meta.yaml file.

The home URL should probably use https instead of http

home: http://github.com/OceanParcels/parcels

Probably we should use the following URL instead:

home: https://github.com/OceanParcels/parcels

In addition, we probably should modify as well the doc_url

doc_url: http://oceanparcels.org/

Probably we should use the following URL instead:

doc_url: https://oceanparcels.org/

@ngam
Copy link
Contributor Author

ngam commented Jul 6, 2023

Thanks @varlackc, fixed!

Copy link
Contributor

@erikvansebille erikvansebille 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, thanks for all this effort! However, I would also like to see @willirath weigh in, who was the original creator of this condo recipe.

@erikvansebille
Copy link
Contributor

Actually, one question: I do see a check for Windows (parcels-feedstock (win win_64_)) and for linux (parcels-feedstock (linux linux_64_)), but not for macOS. Is that intentional? Should we not also check macOS?

@ocefpaf
Copy link
Member

ocefpaf commented Jul 6, 2023

@ocefpaf we could do without outputs.

You are correct, with the modern "semi-arch" we can achieve better results.

@ngam
Copy link
Contributor Author

ngam commented Jul 6, 2023

... but not for macOS. Is that intentional? Should we not also check macOS?

We don't have to because unix encompasses osx; the package itself is noarch, but we made it slightly more complicated than one CI because we want to ensure the runtime compilers are correct. For osx and linux, you're choosing the standard C compilers at runtime (so we can get any of them with a generic c-compiler), but for windows you're choosing a non-default compiler (so we have to be specify it). We can test this further if you'd like before we merge.

See here for the attributes of the package and how it'd look:

# To have conda build upload to anaconda.org automatically, use
# conda config --set anaconda_upload yes
anaconda upload \
    /home/conda/feedstock_root/build_artifacts/noarch/parcels-2.4.2-pyh4c28395_2.conda
anaconda_upload is not set.  Not uploading wheels: []

INFO :: The inputs making up the hashes for the built packages are as follows:
{
  "parcels-2.4.2-pyh4c28395": {
    "recipe": {
      "__unix": "__unix",
      "c_compiler": "gcc",
      "channel_targets": "conda-forge main"
    }
  }
}

# To have conda build upload to anaconda.org automatically, use
# $ conda config --set anaconda_upload yes
anaconda upload ^
    D:\bld\noarch\parcels-2.4.2-pyh7428d3b_2.conda
anaconda_upload is not set.  Not uploading wheels: []

INFO :: The inputs making up the hashes for the built packages are as follows:
{
  "parcels-2.4.2-pyh7428d3b": {
    "recipe": {
      "__win": "__win",
      "channel_targets": "conda-forge main"
    }
  }
}

In both cases, the package is noarch, but in the first, there is a conditional on __unix (meaning it will only be installable on unix machines, e.g. linux64, linux aarch64, osx, etc.). Likewise, for he second, you see the conditional on __win for windows machines. Note how the package hashes are different (pyh4c28395 vs pyh7428d3b) and that's because of the different runtime dependencies.

@willirath
Copy link
Contributor

Thanks for pinging me. I like the semi-arch idea, but I'm a little sceptical if we really cover the implications of the new way to depend on the compilers
with the current tests.

@willirath
Copy link
Contributor

willirath commented Jul 10, 2023

(ED: Moved to review comment.)

Copy link
Contributor

@willirath willirath left a comment

Choose a reason for hiding this comment

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

As already mentioned in the comments, I'd very much like to explicitly test that compilers are present and found on OSX as well.

recipe/meta.yaml Outdated Show resolved Hide resolved
recipe/meta.yaml Show resolved Hide resolved
@ngam
Copy link
Contributor Author

ngam commented Jul 10, 2023

As already mentioned in the comments, I'd very much like to explicitly test that compilers are present and found on OSX as well.

Let me know if the edits are sufficient. Thanks!

@erikvansebille
Copy link
Contributor

As already mentioned in the comments, I'd very much like to explicitly test that compilers are present and found on OSX as well.

Let me know if the edits are sufficient. Thanks!

Thanks for also adding the osx test, @ngam. Glad to see that the python example_peninsula.py call works with the built-in compilers.

I think this should be fine now to merge this PR; do you agree @willirath?

Copy link

@varlackc varlackc 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 To Me!

@willirath
Copy link
Contributor

Looks good to me as well

@willirath
Copy link
Contributor

No idea how I can formally approve on mobile. Feel free to ignore my review and merge.

@erikvansebille erikvansebille merged commit 50ab3a4 into conda-forge:main Jul 13, 2023
5 checks passed
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