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

classes/cmake: Set CMAKE_SYSTEM_PROCESSOR #102

Closed
wants to merge 0 commits into from

Conversation

Ferruck
Copy link
Contributor

@Ferruck Ferruck commented Jan 18, 2022

Some CMake scripts access this variable unconditionally, such as libjpeg-turbo's. Looking at the official docs they seem to be right in the assumption that CMAKE_SYSTEM_PROCESSOR will always be set.

Content-wise the variable seems to match the first part of the AUTOCONF_HOST triplet for all Unix-ish OSs. I'm not sure how the whole cygwin/mingw-stuff has to be handled here, though, since normally Windows has its own possible settings for CMAKE_SYSTEM_PROCESSOR like AMD64, ARM64 or IA64. Do we need to take those when compiling via cygwin/mingw or just the same first part of the triplet? I'd say that it'll probably make no difference in the majority of cases but I'd rather provide a correct solution, anyways.

@jkloetzke
Copy link
Member

Oh well, looks like another loosely defined variable in build systems. I think your approach is good enough for the time being. It might also work for Windows/MSYS2 because any package using this variable probably needs to handle the Linux cases too.

@mahaase Do you use this class on MSYS2?

@mahaase
Copy link
Contributor

mahaase commented Jan 20, 2022

@mahaase Do you use this class on MSYS2?

yep, currently result is: set(CMAKE_SYSTEM_PROCESSOR x86_64) which matches to https://github.com/BobBuildTool/basement/blob/master/plugins/multiarch.py, doesn't it?

@jkloetzke
Copy link
Member

AFAIU this is correct for the case that MSYS2 applications are built. But for cmake running on native Windows this would be wrong. At least that's my impression if I look at conan-io/conan#8026 (comment).

So the question is if you're building native applications via CMake through this class with the microsoft VS toolchain. If yes, then the values from the linked PR should apply...

@Ferruck
Copy link
Contributor Author

Ferruck commented Jan 21, 2022

So, I've made some changes taking into account what I found in the devel::win::vs2019-toolchain recipe. I don't know what I'm doing here, though (the linked issue just made the whole mess more confusing, IMHO), and am thus open for comments. I also have no way of testing this locally, so, @mahaase, it'd be nice if you could give it a shot...

@Ferruck
Copy link
Contributor Author

Ferruck commented Jan 21, 2022

I can just agree with this comment:

Otoh, I searched for CMAKE_SYSTEM_PROCESSOR arm and CMAKE_SYSTEM_PROCESSOR aarch on github and it looks like there is no universal standard.
I think we have to face the reality that there is no universally-agreed-to-standard to use as CMAKE_SYSTEM_PROCESSOR variable.
Like everything in the c/c++ world (except for the stl standaard), there is no standard. CMake allows everything. CMAKE_SYSTEM_PROCESSOR is just a hint.

In reality, some project will either use one kind CMAKE_SYSTEM_PROCESSOR mapping from processor to cmake variable and not switch between a Microsoft convention or GNU convention. Or will not switch between Windows convention or Linux convention.

@jkloetzke
Copy link
Member

I guess this is as good as it can get. As it's only loosely defined we shouldn't put too much effort into this until a problem is found. Unless @mahaase has some objection I'm going to merge it...

@Ferruck
Copy link
Contributor Author

Ferruck commented Jan 27, 2022

Sorry for the noise, seems I'm too dumb for that whole Github flow. I've created a new PR (this time not referencing master 🙄 ) in #111

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.

3 participants