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

Platform target property #1246

Merged
merged 4 commits into from
Jun 24, 2022
Merged

Platform target property #1246

merged 4 commits into from
Jun 24, 2022

Conversation

Soroosh129
Copy link
Contributor

This is based on the work started by @sberkun in #1118.

@@ -0,0 +1,12 @@
/**
* Test that the cross-compile capability works as expected.
Copy link
Member

Choose a reason for hiding this comment

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

What does this test, exactly? Isn't this expected to only work on macOS?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, I'm hoping that this works on Linux too. The target tests for C only run on Mac and Linux.

Copy link
Member

Choose a reason for hiding this comment

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

The target tests for C only run on Mac and Linux.

That's exactly why I'm asking. I would expect compilation to work, but the produced executable to not work on Linux or Windows... If not, when what does the target property do at all?

Copy link
Contributor Author

@Soroosh129 Soroosh129 Jun 23, 2022

Choose a reason for hiding this comment

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

As I commented below, there is a lot more to cross-compiling than just setting the platform. In that sense, the platform target property doesn't enable true cross-compiling on its own as implemented. Essentially, all it does right now is to select which one of the platform files we link against. The platform support files for Mac should work on Linux too, hence why I think the test will pass.

To enable true cross-compiling, we need to add additional missing pieces (marked with ???):

# the name of the target operating system
# set(CMAKE_SYSTEM_NAME Windows) # This is set by the platform target property

# which compilers to use for C and C++
set(CMAKE_C_COMPILER   i586-mingw32msvc-gcc) # ???
set(CMAKE_CXX_COMPILER i586-mingw32msvc-g++) # ???

# where is the target environment located
set(CMAKE_FIND_ROOT_PATH  /usr/i586-mingw32msvc
    /home/alex/mingw-install) # ???

# adjust the default behavior of the FIND_XXX() commands:
# search programs in the host environment
set(CMAKE_FIND_ROOT_PATH_MODE_PROGRAM NEVER) # ???

# search headers and libraries in the target environment
set(CMAKE_FIND_ROOT_PATH_MODE_LIBRARY ONLY) # ???
set(CMAKE_FIND_ROOT_PATH_MODE_INCLUDE ONLY) # ???

Copy link
Member

Choose a reason for hiding this comment

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

Got it. What do we have to do to support a CMAKE_SYSTEM_NAME that is not a popular OS but something like Arduino or FlexPRET? I did find https://github.com/arduino-cmake/Arduino-CMake-NG.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is very promising. I think if we want to use this, from the looks of the example in the README of that repo, we need to generate a custom CMakeLists.txt entirely for Arduino since executables are created by calling a add_arduino_executable method, which is a custom CMake method.

Copy link
Member

Choose a reason for hiding this comment

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

Tagging @arengarajan99 for ☝️

@Soroosh129
Copy link
Contributor Author

It should be noted that the platform target property is only part of the picture. There is more to cross-compiling something than just setting the platform.

Copy link
Member

@lhstrh lhstrh left a comment

Choose a reason for hiding this comment

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

This looks great, but I'm not sure about the test...

@Soroosh129
Copy link
Contributor Author

Soroosh129 commented Jun 23, 2022

This looks great, but I'm not sure about the test...

I'm not sure either. I'm 51% sure it works on Linux too :). Let's wait for the CI to finish.

@Soroosh129 Soroosh129 marked this pull request as ready for review June 23, 2022 07:32
@lhstrh
Copy link
Member

lhstrh commented Jun 24, 2022

Looks like this is a go?

@lhstrh lhstrh merged commit e1081bc into master Jun 24, 2022
@lhstrh lhstrh deleted the platform-target-property branch June 24, 2022 01:22
@Soroosh129 Soroosh129 added enhancement Enhancement of existing feature c Related to C target python Related to the Python target labels Jul 1, 2022
@lhstrh lhstrh added feature New feature and removed enhancement Enhancement of existing feature labels Jul 7, 2022
@lhstrh lhstrh changed the title Add platform target property Added platform target property Jul 7, 2022
@lhstrh lhstrh changed the title Added platform target property Platform target property Jul 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c Related to C target feature New feature python Related to the Python target
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants