Skip to content

Adds a new os.targetarch() function. #2263

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

Merged
merged 3 commits into from
Sep 28, 2024
Merged

Conversation

tritao
Copy link
Contributor

@tritao tritao commented Sep 26, 2024

What does this PR do?

This PR introduces the concept of a target architecture _TARGET_ARCH, analogous to the existing _TARGET_OS.

So in all my previous Premake projects over the years, I have ended up hacking something comparable to this concept of a target architecture, but it really should be a core Premake concept IMHO.

How does this PR change Premake's behavior?

There is another change that I think should also be done, to default to the target architecture by default. At the moment Premake will default to 32-bits architecture, at least on Windows for example. This behaviour should be updated to target the architecture of the running system by default.

I left that line commented in the PR (in oven.lua), would like to understand what's the community's opinion on that before making further changes as I'll need to update some tests.

Anything else we should know?

I'll also add some new tests specifically for the new APIs if people are ok with this change.

@tritao tritao force-pushed the target-arch branch 3 times, most recently from 60a92dd to ce5ff07 Compare September 26, 2024 14:49
@samsinsane
Copy link
Member

So, I can understand how this would work for a workspace that doesn't specify any platforms or architecture, but how does this work for workspaces that do? Additionally, how does this work for workspaces that specify multiple platforms or architectures?

There is another change that I think should also be done, to default to the target architecture by default. At the moment Premake will default to 32-bits architecture, at least on Windows for example. This behaviour should be updated to target the architecture of the running system by default.

How do you expect this to work with the workspaces that specify their platforms or architecture(s)?

@tritao
Copy link
Contributor Author

tritao commented Sep 26, 2024

So, I can understand how this would work for a workspace that doesn't specify any platforms or architecture, but how does this work for workspaces that do?

In this case I think it should work like it currently does with system and os.target(), in that you have a default value that is assumed, but you can override it with system. In current Premake, you can also set --os but pass an explicit value to system in your configs. The same would be true with --arch and architecture.

Additionally, how does this work for workspaces that specify multiple platforms or architectures?

I would expect those cases to keep working as they have. From what I understand, platform just introduces another axis of configuration, so if you set an explicit architecture on those, it should be automatically picked by Premake during generation (it's been a while since I've used platforms though, so I could be wrong on my assumptions here).

How do you expect this to work with the workspaces that specify their platforms or architecture(s)?

The only "edge case" I am thinking about is if you have say debug and release configs, and x86 and x64 platforms.

Lets say you set an explicit architecture with filter { "platforms:x86/x64" }. Presently, architecture would be nil by default for the debug/release configs, but with the additional change I proposed, it would be set to the target architecture of the current system. This is actually something that can already be constructed with current Premake, and I would expect the platforms architecture to override the one from configurations.

Tests can be added for those edge cases, but generally, I think behavior should just continue to work if my assumptions are correct.

@tritao
Copy link
Contributor Author

tritao commented Sep 26, 2024

To note, CMake also provides this: https://cmake.org/cmake/help/latest/variable/CMAKE_SYSTEM_PROCESSOR.html

@tritao
Copy link
Contributor Author

tritao commented Sep 27, 2024

Ok, so did a bit more experimentation on this and just a few thoughts.

Defaulting to the architecture of the running system leads to some important changes of behavior.

Currently, if you don't set an explicit architecture, then Premake will default to not using any target architecture flags in the toolsets. So for example, you will not get any of either -m32/-m64 flags. However if you do set an explicit architecture, then you do get those flags.

And to note, if you were to set an explicit of architecture of lets say ARM and use the system clang (which is quite capable of cross compilation), then Premake would not correctly pass the target flags.

I'm not really if I find this difference of behaviour depending if you set an explicit architecture or not to be the most robust by Premake, seems like being as most explicit about it would be best, but since it's a pre-existing behaviour, I think it's reasonable to keep it as-is.

As such, I have updated the PR to default _TARGET_ARCH to nil by default, so behavior will only change if it's set either by an user script or with the --arch CLI option.

Any thoughts on this @samsinsane / anyone else?

@samsinsane
Copy link
Member

In this case I think it should work like it currently does with system and os.target(), in that you have a default value that is assumed, but you can override it with system. In current Premake, you can also set --os but pass an explicit value to system in your configs. The same would be true with --arch and architecture.

This sounds good, but doesn't quite answer my question because I don't know if --os has priority over system or not. 😆 Do you happen to know?

seems like being as most explicit about it would be best, but since it's a pre-existing behaviour, I think it's reasonable to keep it as-is.

I agree with being explicit, but there is also a requirement for files generated by Premake to be shared by default. The only limit on this currently is the target OS, you can't go from Windows to Linux unless you specify --os=linux. I remember a few occasions that @starkos mentioned sharing the files was a requirement, but I think most of these were related to people trying to introduce changes like having links search for the libraries and error if they're not found. Obviously, that would prevent you from being able to generate a Linux Makefile from Windows since it wouldn't have access to the system libraries by default.

I think your approach to maintaining the existing behaviour is a good call for now.

@tritao
Copy link
Contributor Author

tritao commented Sep 27, 2024

This sounds good, but doesn't quite answer my question because I don't know if --os has priority over system or not. 😆 Do you happen to know?

If there is a platform, then it has priority, otherwise _OPTIONS.os and last _TARGET_OS.

function os.target()
    return _OPTIONS.os or _TARGET_OS
end
local system = os.target()
local architecture = os.targetarch()
local toolset = p.action.current().toolset

if platform then
	system = p.api.checkValue(p.fields.system, platform) or system
	architecture = p.api.checkValue(p.fields.architecture, platform) or architecture
	toolset = p.api.checkValue(p.fields.toolset, platform) or toolset
end

So behaviour is exactly the same as how system/--os currently works, except _TARGET_ARCH is nil by default due to the behaviour discussed in the previous message.

@tritao tritao marked this pull request as ready for review September 27, 2024 18:12
@tritao
Copy link
Contributor Author

tritao commented Sep 27, 2024

Added some tests and improved the docs, should be ready for review now.

Copy link
Member

@nickclark2016 nickclark2016 left a comment

Choose a reason for hiding this comment

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

Please hold on merging until the questions are addressed

@tritao
Copy link
Contributor Author

tritao commented Sep 28, 2024

Please hold on merging until the questions are addressed

Updated the PR with a fix for the issue brought up by @samsinsane, otherwise I think all questions should be addressed.

@samsinsane samsinsane merged commit 6d3c962 into premake:master Sep 28, 2024
14 checks passed
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