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.

Sign up for GitHub

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

Jump to bottom

jfalcou-eve/v2021.10.0 #7744

Merged
merged 9 commits into from
Oct 23, 2021
Merged

jfalcou-eve/v2021.10.0 #7744

merged 9 commits into from
Oct 23, 2021

Conversation

ericLemanissier
Copy link
Contributor

@ericLemanissier ericLemanissier commented Oct 18, 2021 •
edited
Loading

fixes #7743

Specify library name and version: jfalcou-eve/v2021.10.0

  • I've read the guidelines for contributing.
  • I've followed the PEP8 style guides for Python code in the recipes.
  • I've used the latest Conan client version.
  • I've tried at least one configuration locally with the
    conan-center hook activated.

@conan-center-bot

This comment has been minimized.

Sign in to view
@conan-center-bot conan-center-bot added the Bump version PR bumping version without recipe modifications label Oct 18, 2021
@conan-center-bot

This comment has been minimized.

Sign in to view
@conan-center-bot

This comment has been minimized.

Sign in to view
@ericLemanissier ericLemanissier mentioned this pull request Oct 18, 2021
Closed
@ericLemanissier
Copy link
Contributor Author

ok, so this is blocked by #7529 (waiting for conan-io/conan-docker-tools#320 to be deployed)

@uilianries
Copy link
Member

@ericLemanissier Do you mean we need to update CLang 10 too? That PR was related to Clang 11 only. This PR here is failing with Clang 10.

@ericLemanissier
Copy link
Contributor Author

err, no, I just misread the version number. some more investigation is needed then!

@ericLemanissier
Copy link
Contributor Author

ok, so clang 10 support for concepts seems to be limited: https://groups.google.com/g/compiler-explorer-discussion/c/gwkLj-o-R_8?pli=1

@ericLemanissier
disable clang 10
343b171 
it does not support the library part of concepts
@conan-center-bot conan-center-bot removed the Bump version PR bumping version without recipe modifications label Oct 18, 2021
@conan-center-bot

This comment has been minimized.

Sign in to view
@uilianries
Copy link
Member

ok, so clang 10 support for concepts seems to be limited: groups.google.com/g/compiler-explorer-discussion/c/gwkLj-o-R_8?pli=1

Thank you for checking! ❤️

@conan-center-bot

This comment has been minimized.

Sign in to view
jfalcou
jfalcou reviewed Oct 18, 2021
View reviewed changes
Copy link
Contributor

@jfalcou jfalcou left a comment

Choose a reason for hiding this comment

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

I gave some infos on the minimal setup required.

recipes/jfalcou-eve/all/conanfile.py Show resolved Hide resolved
recipes/jfalcou-eve/all/conanfile.py Outdated Show resolved Hide resolved
@conan-center-bot

This comment has been minimized.

Sign in to view
@conan-center-bot

This comment has been minimized.

Sign in to view
@conan-center-bot

This comment has been minimized.

Sign in to view
@ericLemanissier
Copy link
Contributor Author

@jfalcou do you have an idea about this error https://c3i.jfrog.io/c3i/misc/logs/pr/7744/8-configs/windows-visual_studio/jfalcou-eve/cci.20210823/5ab84d6acfe1f23c4fae0ab88f26e3a396351ac9-test.txt ? (it's cci.20210823, not v2021.10.0)

@jfalcou
Copy link
Contributor

jfalcou commented Oct 19, 2021

@jfalcou do you have an idea about this error https://c3i.jfrog.io/c3i/misc/logs/pr/7744/8-configs/windows-visual_studio/jfalcou-eve/cci.20210823/5ab84d6acfe1f23c4fae0ab88f26e3a396351ac9-test.txt ? (it's cci.20210823, not v2021.10.0)

It makes literally no sense. I suspect MSVC is getting confused.

My surprise is that the recipe even worked with MSVC at all as we don't really support it yet.
I guess an easy fix is to not support MSVC but it may cause issues right?

@ericLemanissier
Copy link
Contributor Author

Well, windows and mac builds were never tested on conan center index: https://c3i.jfrog.io/c3i/misc/summary.html?json=https://c3i.jfrog.io/c3i/misc/logs/pr/6902/14-production/jfalcou-eve/cci.20210823//summary.json
Conan Center Index used to test header-only recipes on linux only, but now it tries all the platforms. I'll simply disable msvc builds.

@jfalcou
Copy link
Contributor

jfalcou commented Oct 19, 2021

Well, windows and mac builds were never tested on conan center index: https://c3i.jfrog.io/c3i/misc/summary.html?json=https://c3i.jfrog.io/c3i/misc/logs/pr/6902/14-production/jfalcou-eve/cci.20210823//summary.json Conan Center Index used to test header-only recipes on linux only, but now it tries all the platforms. I'll simply disable msvc builds.

Seems the safest. We will have msvc-clang support soonish but it'll be for 2021.11.x at least. Not sure CCI test this setup.

@conan-center-bot

This comment has been minimized.

Sign in to view
@ericLemanissier
Copy link
Contributor Author

ericLemanissier commented Oct 19, 2021 •
edited
Loading

😆 all builds are INVALID_CONFIGURATION, I messed up something !

EDIT: EVE supports only gcc 11 and clang 12,which are not yet on CCI (https://github.com/conan-io/conan-center-index/blob/master/docs/supported_platforms_and_configurations.md), this explains that. It's quite risky to put an untested package on CCI, what do you (approvers) think ?

@prince-chrismc prince-chrismc mentioned this pull request Oct 19, 2021
Open
@jfalcou
Copy link
Contributor

jfalcou commented Oct 19, 2021

So question: how does it ended up there first then ?

@ericLemanissier
Copy link
Contributor Author

gcc 10 passed for falcou-eve/cci.20210823 (https://c3i.jfrog.io/c3i/misc/summary.html?json=https://c3i.jfrog.io/c3i/misc/logs/pr/6902/14-production/jfalcou-eve/cci.20210823//summary.json) but you said gcc 10 is not compatible with jfalcou-eve/v2021.10.0

@jfalcou
Copy link
Contributor

jfalcou commented Oct 19, 2021

gcc 10 passed for falcou-eve/cci.20210823 (https://c3i.jfrog.io/c3i/misc/summary.html?json=https://c3i.jfrog.io/c3i/misc/logs/pr/6902/14-production/jfalcou-eve/cci.20210823//summary.json) but you said gcc 10 is not compatible with jfalcou-eve/v2021.10.0

It probably works. clang before 12 is a clear no-no as all the concepts and some std like bit_cast are not there properly (same for libc++ based clang that still lacks some basic things). So, letting the GCC requirement down to 10 is probably OK.

So, I feel bad now to have let you spend all this time on this. Maybe we should lt the recipes "as it is" for now and try again when the CCI uses newer compilers ? I'm unsure of what to do.

@conan-center-bot
Copy link
Collaborator

All green in build 10 (a4dec57aee93bdfc861fc72f352fdd9c4e5184d5):

  • jfalcou-eve/cci.20210823@:
    All packages built successfully! (All logs)

  • jfalcou-eve/v2021.10.0@:
    All packages built successfully! (All logs)

@ericLemanissier
Copy link
Contributor Author

no problem, don't worry. the bot successfully tested both versions of eve on GCC 10, so I guess we're good to go !

@SSE4 SSE4 requested a review from uilianries October 20, 2021 09:37
jgsogo
jgsogo suggested changes Oct 22, 2021
View reviewed changes
Copy link
Contributor

@jgsogo jgsogo left a comment

Choose a reason for hiding this comment

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

Blocking it, just to consider if we should change the version name.

recipes/jfalcou-eve/all/conandata.yml
@@ -2,3 +2,6 @@ sources:
"cci.20210823":
url: "https://github.com/jfalcou/eve/archive/30e7a7f6bcc5cf524a6c2cc624234148eee847be.tar.gz"
sha256: "c267135f7215197ef6859b2aa1c5b6a7fc45ca8333933beda56c96b0d0400ef1"
"v2021.10.0":
Copy link
Contributor

Choose a reason for hiding this comment

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

If we get rid of the v, it will follow semver and version ranges would work here. Do we know if it is semver?

Copy link
Contributor

Choose a reason for hiding this comment

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

Asked here: jfalcou/eve#1027 (comment)

@jgsogo
Copy link
Contributor

jgsogo commented Oct 22, 2021

Confirmed, they will follow pattern vYYYY.MM.PATCH. Shall we discard leading v? (TBH, I cannot recall if leading v was already discarded in the version-ranges algorithm).

@ericLemanissier
Copy link
Contributor Author

I don't mind much, but I think it's simpler to maintain and consume if we use upsteam version names as-is

jgsogo
jgsogo approved these changes Oct 23, 2021
View reviewed changes
Copy link
Contributor

@jgsogo jgsogo left a comment

Choose a reason for hiding this comment

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

Probably the version ranges algorithm is something to revisit, it's not the first time something related to it appears in some comment (related #5651). To be honest, I thought we usually remove the leading v from this kind of version pattern. But, me too, I prefer to use upstream version names as-is.

@conan-center-bot conan-center-bot merged commit eb92a57 into conan-io:master Oct 23, 2021
@ericLemanissier ericLemanissier deleted the patch-10 branch October 23, 2021 13:30
@SpaceIm
Copy link
Contributor

SpaceIm commented Oct 23, 2021

Don't forget that adding v has strong consequences in downstream recipes (don't forget all the mess with spirv-tools).

ivanvurbanov pushed a commit to ivanvurbanov/conan-center-index that referenced this pull request Dec 2, 2021
@ericLemanissier @ivanvurbanov
(conan-io#7744) jfalcou-eve/v2021.10.0
ae6bca3 
* jfalcou-eve/v2021.10.0

fixes conan-io#7743

* Update conandata.yml

* Update conandata.yml

* disable clang 10

it does not support the library part of concepts

* Update conanfile.py

* Update conanfile.py

* Update conanfile.py

* Update conanfile.py

* Update conanfile.py
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jfalcou jfalcou jfalcou left review comments

@uilianries uilianries uilianries approved these changes

@jgsogo jgsogo jgsogo approved these changes

@SSE4 SSE4 SSE4 approved these changes

@memsharded memsharded Awaiting requested review from memsharded

@danimtb danimtb Awaiting requested review from danimtb

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[request] jfalcou-eve/v2021.10.0
7 participants
@ericLemanissier @conan-center-bot @uilianries @jfalcou @jgsogo @SpaceIm @SSE4