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

llvmlite v0.30.0 #21

Merged
merged 10 commits into from
Oct 25, 2019
Merged

Conversation

regro-cf-autotick-bot
Copy link
Contributor

It is very likely that the current package version for this feedstock is out of date.
Notes for merging this PR:

  1. Feel free to push to the bot's branch to update this PR if needed.
  2. The bot will almost always only open one PR per version.
    Checklist before merging this PR:
  • Dependencies have been updated if changed
  • Tests have passed
  • Updated license if changed and license_file is packaged

Note that the bot will stop issuing PRs if more than 3 Version bump PRs generated by the bot are open. If you don't want to package a particular version please close the PR

If this PR was opened in error or needs to be updated please add the bot-rerun label to this PR. The bot will close this PR and schedule another one.

This PR was created by the cf-regro-autotick-bot.
The cf-regro-autotick-bot is a service to automatically track the dependency graph, migrate packages, and propose package version updates for conda-forge. If you would like a local version of this bot, you might consider using rever. Rever is a tool for automating software releases and forms the backbone of the bot's conda-forge PRing capability. Rever is both conda (conda install -c conda-forge rever) and pip (pip install re-ver) installable.
Finally, feel free to drop us a line if there are any issues!

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

@jakirkham
Copy link
Member

@isuruf, it looks like LLVM packaging has undergone some restructuring. How should we be depending on it in llvmlite? In particular we are now seeing an libllvmlite.so dependency is missing.

@isuruf
Copy link
Member

isuruf commented Oct 17, 2019

You should also depend on llvm in host which would add the correct run_exports. llvmdev doesn't add a run_exports since packages usually link statically, but looks like llvmlite is happy with shared linking.

@jakirkham
Copy link
Member

@stuartarchibald, is it ok if we start using dynamic linking to build llvmlite or does that cause issues?

isuruf
isuruf previously requested changes Oct 17, 2019
Copy link
Member

@isuruf isuruf left a comment

Choose a reason for hiding this comment

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

Comment on meta.yaml's Line 23 does not apply anymore. You can depend on compiler('c') in OSX too.

@stuartarchibald
Copy link

@stuartarchibald, is it ok if we start using dynamic linking to build llvmlite or does that cause issues?

Technically it may be ok. However this should probably be considered: http://llvmlite.pydata.org/en/latest/admin-guide/install.html#why-static-linking-to-llvm, I would not anticipate Numba mainline switching to dynamic.

@jakirkham
Copy link
Member

@isuruf, given that it sounds like Numba developers prefer static linking here, how can we force that to happen?

@isuruf
Copy link
Member

isuruf commented Oct 21, 2019

None of the conditions in the linked article apply to us. Nevertheless if you want to link with the static libraries, you have to modify the cmake files in llvmlite to link statically when both static ones and dynamic are available.

@jakirkham
Copy link
Member

The install size concern applies though, right? Agree that the other points don't matter to us.

@jakirkham
Copy link
Member

Does this test failure meaning anything to you @stuartarchibald?

======================================================================
FAIL: test_linux (llvmlite.tests.test_binding.TestDependencies)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/conda/feedstock_root/build_artifacts/llvmlite_1571689347008/_test_env_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placeho/lib/python3.7/site-packages/llvmlite/tests/test_binding.py", line 239, in test_linux
    self.fail("failed parsing dependencies? got %r" % (deps,))
AssertionError: failed parsing dependencies? got {'libgcc_s', 'libLLVM-8', 'ld-linux-x86-64', 'libc'}

----------------------------------------------------------------------

ref: https://dev.azure.com/conda-forge/feedstock-builds/_build/results?buildId=82974&view=logs

@isuruf
Copy link
Member

isuruf commented Oct 21, 2019

The install size concern applies though, right? Agree that the other points don't matter to us.

True, but that concern is true for all other conda-forge packages and we are happy with dynamic linking in them.

@isuruf
Copy link
Member

isuruf commented Oct 21, 2019

That test doesn't matter. when statically linking with llvm, libpthread is also linked in, which is not the case when dynamically linking and the test is especting libpthread to be linked in.

@stuartarchibald
Copy link

Does this test failure meaning anything to you @stuartarchibald?

======================================================================
FAIL: test_linux (llvmlite.tests.test_binding.TestDependencies)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/conda/feedstock_root/build_artifacts/llvmlite_1571689347008/_test_env_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placeho/lib/python3.7/site-packages/llvmlite/tests/test_binding.py", line 239, in test_linux
    self.fail("failed parsing dependencies? got %r" % (deps,))
AssertionError: failed parsing dependencies? got {'libgcc_s', 'libLLVM-8', 'ld-linux-x86-64', 'libc'}

----------------------------------------------------------------------

ref: https://dev.azure.com/conda-forge/feedstock-builds/_build/results?buildId=82974&view=logs

I think this would be an expected fail as CF is not building llvmlite in the manner prescribed by the docs, you may wish to patch this with a fix or patch to ignore.

This test is based around static linking of LLVM, which conda-forge no
longer does. So just patch out this test in the conda-forge build.
recipe/build.sh Outdated Show resolved Hide resolved
@jakirkham
Copy link
Member

We are going to give dynamic linking a go, @stuartarchibald. Hope that is ok. This is more inline with how we building things in conda-forge anyways. If it becomes problematic, happy to revisit.

@jakirkham
Copy link
Member

@isuruf, did I get all of your requested changes? GitHub is showing that there may be something I missed. Though am not seeing it atm.

- """
-
- @unittest.skipUnless(sys.platform.startswith('linux'), "Linux-specific test")
- @unittest.skipUnless(os.environ.get('LLVMLITE_DIST_TEST'), "Distribution-specific test")
Copy link
Member

Choose a reason for hiding this comment

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

Just noticed this. If you remove https://github.com/conda-forge/llvmlite-feedstock/blob/master/recipe/run_test.py#L5, then this test will be skipped.

Copy link
Member

Choose a reason for hiding this comment

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

Good catch. Appears to only be used for this test too. Will use that instead. Thanks.

Copy link
Member

Choose a reason for hiding this comment

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

I left it in, but set it to an empty string. That way if Anaconda or other care about enabling this again, they can do that easily.

These tests only check that `llvmlite` was statically linked. As we are
no longer statically, go ahead and skip these tests and explain why. Try
to keep it easy for others to change this option if they need to.
@jakirkham jakirkham merged commit 1fa3ebd into conda-forge:master Oct 25, 2019
@@ -33,14 +28,13 @@ requirements:
host:
- python
- llvmdev 8.0.*
- llvm 8.0.*
- enum34 # [py27]
# llvmdev is built with libz compression support
- zlib # [unix]
Copy link
Member

Choose a reason for hiding this comment

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

Actually do we still need this?

Copy link
Member

Choose a reason for hiding this comment

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

No

@@ -33,14 +28,13 @@ requirements:
host:
- python
- llvmdev 8.0.*
Copy link
Member

Choose a reason for hiding this comment

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

Should this have been dropped?

Copy link
Member

Choose a reason for hiding this comment

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

Does llvmlite support 9.0.0 ?

Copy link
Member

Choose a reason for hiding this comment

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

Was more asking as there is llvmdev and llvm listed here. Though I might just be unaware of how the split was done.

Copy link
Member

Choose a reason for hiding this comment

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

llvm is a dependency of llvmdev. Only llvm has the run_exports because people use llvmdev for static linking.

Copy link
Member

Choose a reason for hiding this comment

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

llvmdev is needed for the cmake files and headers

Copy link
Member

Choose a reason for hiding this comment

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

Ah ok. Thanks for explaining that. 🙂

Choose a reason for hiding this comment

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

Does llvmlite support 9.0.0 ?

Unknown at present, it's scheduled for testing.

@jakirkham
Copy link
Member

Sorry had a couple more questions that I only thought of after. Can update things if needed.

@regro-cf-autotick-bot regro-cf-autotick-bot deleted the 0.30.0 branch October 25, 2019 04:11
@stuartarchibald
Copy link

We are going to give dynamic linking a go, @stuartarchibald. Hope that is ok. This is more inline with how we building things in conda-forge anyways. If it becomes problematic, happy to revisit.

Sure, if that matches your use case better then it's worth a try. I cannot comment on how well it'll work in practice, would be interested in hearing of any notable findings, thanks!

@jakirkham
Copy link
Member

Of course! Happy to share how things go 😄

@jakirkham
Copy link
Member

Also thanks for your help here. 🙂

@stuartarchibald
Copy link

No problem, thanks for getting the build done!

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.

5 participants