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

Rebuild 5.x for new compiler infrastructure #40

Merged
merged 2 commits into from
Nov 5, 2018

Conversation

SylvainCorlay
Copy link
Member

@SylvainCorlay SylvainCorlay commented Oct 26, 2018

Unlike clang 6.x and 7.x, this makes two variants for the clangdev package:

  • one with the cling patches,
  • and one without

however, this should not use build features at all, and the "cling" flavor should be selected through the build string.

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

@SylvainCorlay
Copy link
Member Author

Hum, it seems that the CLANG_VARIANT environment variable is not being set now.

@isuruf
Copy link
Member

isuruf commented Oct 26, 2018

@SylvainCorlay
Copy link
Member Author

I think the remaining question is how exactly to compose the build string:

  • nothing for the default and "cling" for the cling variant, or also "default" for the default?
  • build number before the string or after...

@SylvainCorlay SylvainCorlay force-pushed the rebuild-5 branch 3 times, most recently from f6e414b to fa8b0c0 Compare October 29, 2018 13:48
@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.

@SylvainCorlay
Copy link
Member Author

cc @msarahan the last commit implement the variant selection for clangdev.

  • The clangdev recipe has a new outputs section with a clang_variant mutex package.

    Non-default version of clang_variant track the non_default_clang feature (which does not really exist and used to weigh down non-default variants of the package, allowing the default variant to take precedence unless required explicitly).

    outputs:
    
      # Mutex package to keep only one variant of clangdev in a given environment.
      #
      # Non-default variants track the "non_default_clang" feature.
      # This is used to weigh down non-default variants of the package, allowing
      # the default variant to take precedence unless required explicitly.
      - name: clang_variant
        version: 1.0
        build:
          string: {{ clang_variant }}
    {% if clang_variant != "default" %}
          track_features:
            - non_default_clang
    {% endif %}
  • The clangdev recipe does not have a specific build string.

  • It has a runtime dependency on clang_variant * {{ clang_variant }}.

  • cling will select the variant by having a host dependency section including

    - clangdev
    - clang_variant * {{ cling }}
    

@msarahan
Copy link
Member

msarahan commented Oct 29, 2018 via email

@SylvainCorlay
Copy link
Member Author

SylvainCorlay commented Oct 29, 2018

Seems right. I hope it works. Let me know if it does not.

Thanks, does not seem to work quite yet. Maybe I need to add an entry to outputs for clangdev itself.

Packaging clangdev
INFO:conda_build.build:Packaging clangdev
No files or script found for output clangdev
WARNING:conda_build.build:No files or script found for output clangdev

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

I do have some suggestions for making it better though...

For recipe:

  • It looks like the 'clang_variant' output doesn't have any tests.

@SylvainCorlay SylvainCorlay changed the title WIP: Rebuild 5.x for new compiler infrastructure Rebuild 5.x for new compiler infrastructure Nov 5, 2018
@SylvainCorlay
Copy link
Member Author

@isuruf this seems to work well. It is the first time I make use of subpackages and mutex packages.

I know that you are interested in cling for the symengine demos, and this is the last PR to merge before we tackle build per se.

string: {{ clang_variant }}
{% if clang_variant != "default" %}
track_features:
- non_default_clang
Copy link
Member

Choose a reason for hiding this comment

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

Would it be possible to not use track_features here?

Copy link
Member Author

Choose a reason for hiding this comment

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

In fact there is no such feature, and no package has a features section.

This was suggested by @msarahan on gitter. The track_features statement is just meant to weigh down the cling variant so that it is not installed by default.

This is used e.g. in the default channel for the selection of openblas vs mkl:
https://github.com/AnacondaRecipes/openblas-feedstock/blob/master/recipe/meta.yaml#L136

Copy link

Choose a reason for hiding this comment

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

Right, this "unmatched" track_features approach is now our best approach for specifying a variant preference. Because it doesn't have an accompanying features it avoids the messiness.

Copy link

Choose a reason for hiding this comment

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

How do you anticipate selecting the non-default variant? By pinning clang_variant=*=cling? My recommendation is to create a metapackage that depends on clang_variant=*=cling that users can install. That's a cleaner user experience than pinning, in my view.

Copy link
Member Author

Choose a reason for hiding this comment

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

I was thinking of cling itself doing clang_variant=*=cling in its runtime dependencies. I can't think of a situation where we would want this variant and not cling.

Copy link
Member Author

Choose a reason for hiding this comment

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

Right, this "unmatched" track_features approach is now our best approach for specifying a variant preference. Because it doesn't have an accompanying features it avoids the messiness.

Since features are meant to disappear, it could be interesting to replace the unmatched track_features approach with some other functionally equivalent low_precedence: true thingy.

Copy link
Member

Choose a reason for hiding this comment

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

...or better yet precedence: <some number> 😉

@jakirkham
Copy link
Member

Would you have time to look this one over, @mcg1969? This is a new attempt at handling selection between two clangdev variants.

@@ -1,8 +1,7 @@
{% set version = "5.0.0" %}
{% set sha256 = "019f23c2192df793ac746595e94a403908749f8e0c484b403476d2611dd20970" %}

{% set clang_variant = os.environ.get('CLANG_VARIANT', 'default') %}
{% set build_number = "0" %}
{% set build_number = "1000" %}
Copy link

Choose a reason for hiding this comment

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

I'm surprised this is still happening. What benefit does it still offer?

Copy link
Member Author

Choose a reason for hiding this comment

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

The build_number = "1000" statement is for the new compilers rebuild of conda-forge.

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.

6 participants