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

Update jfalcou-eve requirements as of v2022.03.0 #9785

Merged
merged 12 commits into from
Aug 9, 2022

Conversation

jfalcou
Copy link
Contributor

@jfalcou jfalcou commented Mar 15, 2022

jfalcou-eve/v2022.03.0

Latest version of EVE now requires clang 13 and g++ 11 on Linux


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

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.

If it is only required for certain versions, it should be conditioned to those versions.

@conan-center-bot

This comment has been minimized.

@jfalcou
Copy link
Contributor Author

jfalcou commented Mar 24, 2022

Is there something I should do to fix those issues?

@jgsogo
Copy link
Contributor

jgsogo commented Mar 24, 2022

it looks like some missing c++ standard feature, right?

/Users/jenkins/w/prod/BuildSingleReference@2/.conan/data/jfalcou-eve/v2021.10.0/_/_/package/5ab84d6acfe1f23c4fae0ab88f26e3a396351ac9/include/eve/concept/vectorized.hpp:46:62: error: no member named 'integral' in namespace 'std'; did you mean 'internal'?
  concept integral_simd_value        = simd_value<T> && std::integral<detail::value_type_t<T>>;
                                                        ~~~~~^

Either apple-clang 13.0 doesn't support it, or you need to activate that standard in the build-system files

@jfalcou
Copy link
Contributor Author

jfalcou commented Mar 24, 2022

Well, the previous recipe worked before so that's rather strange.
Same for MSVC, I thought the previous recipe deactivated them cause we just don't support them.

@jfalcou
Copy link
Contributor Author

jfalcou commented Mar 24, 2022

or should I remove the msvc apple-clang line entirely?

@jgsogo
Copy link
Contributor

jgsogo commented Mar 24, 2022

The latest PR to this recipe is 5 months old, by that time apple-clang 13 was not used here. So this is the first time this recipe tries this compiler. As this is a newer version for apple-clang I would recommend investigating the failure instead of ignoring it. Apple-clang 13 is the default in the macos world now.

@jfalcou
Copy link
Contributor Author

jfalcou commented Mar 24, 2022

Yes maybe.
The thing is, I don't have an apple machine to do such a test but it looks like apple-clang just doesn't have the correct libc++ for C++20. So I don't see what I could do but not support apple-clang till they update their compiler stack.

For me it's the same than not working with sub-par MSVC. The library just doesn't support it.

@jgsogo
Copy link
Contributor

jgsogo commented Mar 24, 2022

You are the author of the library, so you are the best one to declare if the library is supposed to work or not with those compilers. Probably the best way to go is to skip here that compiler-version and then modify the sources to support it and, with a new release of the library, enable those compiler-versions here again.

@jfalcou
Copy link
Contributor Author

jfalcou commented Mar 24, 2022

OK. So I just remove the apple-clang entry then ?

@jgsogo
Copy link
Contributor

jgsogo commented Mar 24, 2022

But still I don´t understand the purpose of this PR. The new compiler-version requirements apply to v2022.03.0 version, which is not added.

You should use something a bit more complex (pseudocode):

def validate(self):
   if self.version in ['cci.20210823', 'v2021.10.0']:
      # Check for supported compilers (the ones already in the recipe)
      
      # Even if apple-clang should be supported (regarding C++ standard), it is isn't
      if self.settings.compiler == ´apple-clang'  and self.settings.compiler.version >= 13:
         raise ConanInvalidConfiguration("Not supported")
         
   if self.version in ['v2022.03.0']:
      # New checks for supported compilers (the ones in this PR), which are more restritive

You don't want people that was using an existing version with some compiler to start getting an error just because new versions doesn´t support that compiler.

@jfalcou
Copy link
Contributor Author

jfalcou commented Mar 24, 2022

the thing is that it never supported apple-clang. The original recipe was not written by me and I try to make it uptodate.
Will give a try to your solution.

@ericLemanissierBot
Copy link

I detected other pull requests that are modifying jfalcou-eve/all recipe:

This message is automatically generated by https://github.com/ericLemanissier/conan-center-conflicting-prs so don't hesitate to report issues/improvements there.

@fpelliccioni
Copy link
Contributor

Hey guys, how can I help for having this PR landed?

@jfalcou
Copy link
Contributor Author

jfalcou commented Apr 5, 2022

We need to make it so it clearly states it doesn't compile on Apple Clang. I didn't have time to patch it.
Be my guest if you want to give it a try, I can help with details.

EDIT: I also missed the folders rules. Will add them

@conan-center-bot

This comment has been minimized.

@jgsogo
Copy link
Contributor

jgsogo commented Apr 7, 2022

You have added version v2022.03.0 in the file config.yml, but you haven't added the corresponding entry to conandata.yml. This is why you get the error:

ERROR: jfalcou-eve/v2022.03.0: Error in source() method, line 62
	tools.get(**self.conan_data["sources"][self.version], strip_root=True,
	KeyError: 'sources'

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@jgsogo
Copy link
Contributor

jgsogo commented Aug 8, 2022

	if  tools.scm.Version(self.version) >= "2022.03.0":
	AttributeError: module 'conan.tools' has no attribute 'scm'

It should work, but it doesn't. It is a bug in current Conan releases, but it is not going to be backported (at least not just because of this bug) as the workaround is pretty straightforward (from conan.tools.scm import Version).

Sorry for the inconvenience.

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@github-actions
Copy link
Contributor

github-actions bot commented Aug 8, 2022

Hooks produced the following warnings for commit 1613bc6
jfalcou-eve/v2022.03.0
post_source(): WARN: [SHORT_PATHS USAGE (KB-H066)] The file './source_subfolder/examples/algorithms/using_existing/inclusive_scan_par_unseq__using_eve_to_build_both_vectorized_and_parallel_algos.cpp' has a very long path and may exceed Windows max path length. Add 'short_paths = True' in your recipe.

@conan-center-bot

This comment has been minimized.

@conan-center-bot
Copy link
Collaborator

All green in build 23 (e559fd40fc095447720316b74072cf05bdfa0682):

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

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

@github-actions
Copy link
Contributor

github-actions bot commented Aug 8, 2022

Hooks produced the following warnings for commit e559fd4
jfalcou-eve/v2022.03.0
post_source(): WARN: [SHORT_PATHS USAGE (KB-H066)] The file './source_subfolder/examples/algorithms/using_existing/inclusive_scan_par_unseq__using_eve_to_build_both_vectorized_and_parallel_algos.cpp' has a very long path and may exceed Windows max path length. Add 'short_paths = True' in your recipe.
jfalcou-eve/v2021.10.0
post_source(): WARN: [SHORT_PATHS USAGE (KB-H066)] The file './source_subfolder/examples/algorithms/using_existing/inclusive_scan_par_unseq__using_eve_to_build_both_vectorized_and_parallel_algos.cpp' has a very long path and may exceed Windows max path length. Add 'short_paths = True' in your recipe.

@prince-chrismc
Copy link
Contributor

Please do not force push 🙏 GitHub forces us to restart the review which is not fun!

from conans.errors import ConanInvalidConfiguration
import os

required_conan_version = ">=1.33.0"
Copy link
Contributor

Choose a reason for hiding this comment

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

The conan v2 API need a higher version (this is not the right one but just better)

Suggested change
required_conan_version = ">=1.33.0"
required_conan_version = ">=1.43.0"

@conan-center-bot conan-center-bot merged commit 6de10d6 into conan-io:master Aug 9, 2022
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

7 participants