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

Add pyjion #17937

Merged
merged 38 commits into from
Feb 13, 2022
Merged

Add pyjion #17937

merged 38 commits into from
Feb 13, 2022

Conversation

dhirschfeld
Copy link
Member

@dhirschfeld dhirschfeld commented Feb 4, 2022

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).
  • If static libraries are linked in, the license of the static library is packaged.
  • 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-linter
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/pyjion) and found some lint.

Here's what I've got...

For recipes/pyjion:

  • Non noarch packages should have python requirement without any version constraints.
  • Non noarch packages should have python requirement without any version constraints.

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

@dhirschfeld
Copy link
Member Author

Note!

This is using a git_url as the sources aren't published on pypi and the build requires submodules to be present.

@dhirschfeld
Copy link
Member Author

dhirschfeld commented Feb 5, 2022

IIUC the pyjion repo builds using the dotnet submodule:

I'm checking with the author what that means in terms of also distributing the .NET license

@conda-forge-linter
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/pyjion) and found some lint.

Here's what I've got...

For recipes/pyjion:

  • The test section contained an unexpected subsection name. commnds is not a valid subsection name.

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

@dhirschfeld
Copy link
Member Author

dhirschfeld commented Feb 6, 2022

It looks like CI is "passing" because all the builds are being skipped?! 🤔

Computed that there are 0 distributions to build from 1 recipes
Nothing to do

@dhirschfeld dhirschfeld marked this pull request as ready for review February 6, 2022 11:39
@dhirschfeld
Copy link
Member Author

I've removed the draft status in case the solution here is to simply merge and debug in the feedstock.

Will see what the experts in @conda-forge/staged-recipes think...

@dhirschfeld
Copy link
Member Author

It seems cmake isn't using ninja. Looks like I need to set CMAKE_MAKE_PROGRAM. That'll have to be a problem for the morning though!

  Running command Building wheel for pyjion (pyproject.toml)


  --------------------------------------------------------------------------------
  -- Trying "Unix Makefiles" generator
  --------------------------------
  ---------------------------
  ----------------------
  -----------------
  ------------
  -------
  --
  Not searching for unused variables given on the command line.
  CMake Error: CMake was unable to find a build program corresponding to "Unix Makefiles".  CMAKE_MAKE_PROGRAM is not set.  You probably need to select a different build tool.
  -- Configuring incomplete, errors occurred!
  See also "/home/conda/staged-recipes/build_artifacts/pyjion_1644155830840/work/_cmake_test_compile/build/CMakeFiles/CMakeOutput.log".
  --
  -------
  ------------
  -----------------
  ----------------------
  ---------------------------
  --------------------------------
  -- Trying "Unix Makefiles" generator - failure
  --------------------------------------------------------------------------------

  ********************************************************************************
  scikit-build could not get a working generator for your system. Aborting build.
  
  × Building wheel for pyjion (pyproject.toml) did not run successfully.
  │ exit code: 1
  ╰─> See above for output.

@dhirschfeld
Copy link
Member Author

The env var seemed to get passed through but still wasn't used:

UserWarning: The environment variable 'CMAKE_GENERATOR' is being passed through with value 'Ninja'.
If you are splitting build and test phases with --no-test, please ensure that this value is also set similarly at test time.

@dhirschfeld
Copy link
Member Author

Odd MacOS error:

==> /Users/runner/mambaforge/bin/git describe --tags --dirty <==

1.2.6

fatal: iconv_open(UTF-8,UTF-8-MAC) failed, but needed:
    precomposed unicode is not supported.
    If you want to use decomposed unicode, run
    "git config core.precomposeunicode false"

fatal: 'git status --porcelain=2' failed in submodule CoreCLR

xref: conda-forge/git-feedstock#50 (comment)

@dhirschfeld dhirschfeld reopened this Feb 7, 2022
@dhirschfeld
Copy link
Member Author

PTAL @conda-forge/staged-recipes - I think this is good to merge!

@dhirschfeld
Copy link
Member Author

There's a lot of top-class, whack-a-mole debugging in this PR - I'd suggest squashing & merging! 😅

recipes/pyjion/meta.yaml Outdated Show resolved Hide resolved
@dhirschfeld
Copy link
Member Author

Thanks @isuruf - it looks like that did the trick!

recipes/pyjion/meta.yaml Outdated Show resolved Hide resolved
@@ -0,0 +1,79 @@
{% set name = "pyjion" %}
{% set version = "1.2.6" %}
Copy link
Member Author

Choose a reason for hiding this comment

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

image

The commit hash (tonybaloney/Pyjion@66a772d) could be used instead if there are concerns the tag could change?

@dhirschfeld
Copy link
Member Author

Also, thanks for the review @chrisburr and @isuruf! I'm happy to go with the multiple sources if that's the decision - I just wanted to explain my reasoning here.

@dhirschfeld
Copy link
Member Author

Actually tested without missing_dso_whitelist (I think I must've Ctrl-Z that change before). It fails without it so I guess I have to add it back:

ERROR (pyjion,lib/python3.10/site-packages/pyjion/_pyjion.cpython-310-x86_64-linux-gnu.so):
$RPATH/libclrjit.so not found in packages, sysroot(s) nor the missing_dso_whitelist
... is this binary repackaging?

The dotnet SDK is a binary repackage so perhaps that's why it's required here? Investigating building dotnet from source is on my TODO list.

Possibly required because `dotnet` is a binary repackage.
@isuruf
Copy link
Member

isuruf commented Feb 8, 2022

The dotnet SDK is a binary repackage so perhaps that's why it's required here? Investigating building dotnet from source is on my TODO list.

No, that's because _pyjion.cpython-310-x86_64-linux-gnu.so is built without the rpath.

@dhirschfeld
Copy link
Member Author

No, that's because _pyjion.cpython-310-x86_64-linux-gnu.so is built without the rpath.

Ok. Is there a way to fix that, or should I just keep the missing_dso_whitelist?

@dhirschfeld
Copy link
Member Author

Ok, re-added missing_dso_whitelist. Build/tests passing. I think I've addressed all the review issues, let me know if you'd like any other changes...

@dhirschfeld
Copy link
Member Author

ping @chrisburr, @isuruf since you're most familiar with this PR. It would be great to get this merged if you're happy with it?

recipes/pyjion/meta.yaml Outdated Show resolved Hide resolved
@dhirschfeld
Copy link
Member Author

dhirschfeld commented Feb 10, 2022

CI passed before so I'm happy for this to be merged as the error is related to the azure outage rather than the code.
xref: conda-forge/status#125

No hosted parallelism has been purchased or granted

@dhirschfeld dhirschfeld reopened this Feb 10, 2022
@dhirschfeld
Copy link
Member Author

dhirschfeld commented Feb 11, 2022

ping @conda-forge/staged-recipes!

@carterbox carterbox merged commit 4d24c2a into conda-forge:main Feb 13, 2022
@dhirschfeld dhirschfeld deleted the pyjion branch February 13, 2022 02:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

5 participants