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

[conan-center-index] Checks for semantic errors #182

Open
prince-chrismc opened this issue Apr 24, 2020 · 9 comments · May be fixed by #184
Open

[conan-center-index] Checks for semantic errors #182

prince-chrismc opened this issue Apr 24, 2020 · 9 comments · May be fixed by #184

Comments

@prince-chrismc
Copy link
Contributor

It would be nice if there was the addition of two new checks, thought I am not sure where they would go in the hooks.

  1. usage of self.requires.add which should not have the last add portion
  2. usage of tools.check_min_cppstd should always have a preceding if self.settings.get_safe("compiler.cppstd"):

I am sure there are other "bad practices" which could be added but these two are very common looking at older recipes, next time they get update it will be more complete.

@prince-chrismc
Copy link
Contributor Author

Seems like #177 helps close this

@prince-chrismc
Copy link
Contributor Author

It also appears the CCI wiki it a bit out of date.

@Croydon
Copy link
Contributor

Croydon commented Apr 24, 2020

What parts of the wiki are outdated?

@uilianries
Copy link
Member

  1. usage of self.requires.add which should not have the last add portion

It's duplicated with #177

@prince-chrismc
Copy link
Contributor Author

"Error-Knowledge-Base" ends at # 37 but there's 44 with the addition of #177

https://github.com/conan-io/conan-center-index/wiki/Error-Knowledge-Base

uilianries added a commit to uilianries/hooks that referenced this issue Apr 24, 2020
@uilianries uilianries linked a pull request Apr 24, 2020 that will close this issue
uilianries added a commit to uilianries/hooks that referenced this issue Apr 24, 2020
@Croydon
Copy link
Contributor

Croydon commented Apr 25, 2020

Probably only 40 is missing

"KB-H040": "NO TARGET NAME",

@SSE4
Copy link
Contributor

SSE4 commented Apr 27, 2020

@uilianries @prince-chrismc could you give me some additional background, e.g. why does tools.check_min_cppstd require if self.settings.get_safe("compiler.cppstd")? per documentation, it's not required.

@prince-chrismc
Copy link
Contributor Author

This comment is why I brought it up over here. lots of discussion around it in the PR

@SSE4
Copy link
Contributor

SSE4 commented Apr 28, 2020

I've been talking with @uilianries about that.
the current thing of adding if self.settings.get_safe("compiler.cppstd") bypasses the check for minimal cppstd if cppstd is None.
for example, given the GCC 4.9 compiler (all the explanation below is for that particular compiler version), the default C++ standard would be C++98. it means cppstd=None implies -std=c++98 for this particular compiler. besides that, GCC 4.9 compiler still may produce C++11 code, if you specify cppstd=11 explicitly, or pass -std=c++11 by other means (e.g. from the build system).
it appears that some projects (don't know the list, but seems to be many of them), unconditionally force -std=c++11 in their build files (Makefiles, CMakeLists, etc.), e.g. by setting CXX_STANDARD 11 or whatever else their build system supports.
for such particular projects, we pass cppstd=None (which implies cppstd=98), but it silently changes in build system to -std=c++11, and you still have a package built.
if you specify tools.check_min_cppstd(11) in recipe, it will not allow the build with cppstd=None for the reasons above, therefore you will get zero packages generated for the given compiler. that's the downside of the current CI model that it only builds with cppstd=None.
recommendation to add if self.settings.get_safe("compiler.cppstd") is temporary workaround to get some packages still generated in the current model of the CI.
however, we're currently reconsidering how to handle cppstd in our CI in general, and I'd say there is high chance that everything will change significantly soon. probably, most of recipes will have to change in order to adapt to the new model.
the current temporary workaround has some issues, and the risk of producing invalid packages.
it means, you're compiling the package with cppstd=None (implies cppstd=98), but it actually silently compiles with -std=c++11, but the package id assigned will still have cppstd=98, which is obviously wrong.
given the API headers like:

#if __cplusplus >= 201103L
std::unique_ptr<engine> get_engine();
#else
#error "C++11 is required to use the library
#endif

you will probably be unable to use the library with cpstd=None at all. moreover, you may encounter runtime issues in the cases like:

#if __cplusplus >= 201103L
std::unique_ptr<engine> get_engine();
#else
std::auto_ptr<engine> get_engine(); // fallback to the old-fashioned code
#endif

the cases like above might be rare and some corner cases, but it's still incorrect to mark c++11 packages with c++98 package id (which we're doing for some packages in conan center right now).
e.g. if certain C++ project requires C++11 to build, but has C API, you're most likely safe to do such things, but then you probably want to just erase the cppstd completely from the package id, as it doesn't matter at all in such a given case.
TL/DR
summary: I don't think it's correct to add that check to the hooks. this thing is a least evil, and a temporary workaround to still generate some packages (potentially invalid) with current CI limitations.

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 a pull request may close this issue.

4 participants