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

rename PremakeDeps to PremakeMultiDeps and add single configuration PremakeDeps #17355

Closed
wants to merge 2 commits into from

Conversation

Enhex
Copy link
Contributor

@Enhex Enhex commented Nov 20, 2024

Changelog: (Feature): because the current Premake generator seems to have problems because of the way it supports multi config (#17345), add a separate single config generator that's simpler and doesn't have config mismatch problems.

Docs: https://github.com/conan-io/docs/pull/XXXX

  • Refer to the issue that supports this Pull Request.
  • If the issue has missing info, explain the purpose/use case/pain/need that covers this Pull Request.
  • I've read the Contributing guide.
  • I've followed the PEP8 style guides for Python code.
  • I've opened another PR in the Conan docs repo to the develop branch, documenting this one.

@CLAassistant
Copy link

CLAassistant commented Nov 20, 2024

CLA assistant check
All committers have signed the CLA.

Copy link
Member

@memsharded memsharded left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution @Enhex

All Conan 2 generators are multi-config when applicable. If PremakeDeps is not, it has to be fixed, the intent is that it is multi-config, and it can be considered a bug if not.

Given the other PR and how it was broken, it is almost guaranteed that nobody was using it at least on Windows, it is very low risk to fix it there.

It would be good to do the improvements on top of #17350, lets try to merge it soon, so you can contribute on top of it.

Thanks again for your contribution!

@Enhex
Copy link
Contributor Author

Enhex commented Nov 20, 2024

I'm not sure how to fix the existing multi-config PremakeDeps.
The generated .lua files should tell the premake5.lua what build type and arch to use,
and the current generator implementation seems to expect premake5.lua to tell it what build type and arch to use,
which creates cyclic dependency.

when generating and using a multi-config build, how do you select which config to use?
Premake can take command line argument for architecture, but it doesn't know which
built type is being used as it leaves the selection to the build tool it generates files for (make/visual studio):
https://premake.github.io/docs/Your-First-Script/

it means that it can't work with the current approach of letting premake5.lua tell conandeps.premake5.lua which build type to use, the generated conandeps.premake5.lua approach of picking a default config doesn't work, and manually calling conan_setup() for every combination of arch and build type is highly undesirable un-automation.

perhaps the author of the current generator can explain how to properly use it.

@memsharded
Copy link
Member

Maybe @Ohjurot can also give some insights into this?

@Ohjurot
Copy link
Contributor

Ohjurot commented Nov 21, 2024

Hello together,

first of all thank you for the feedback the proposed changes!

Maybe first some background information on my inteds when porting the premake generator to conan2:

My main intend was to build a working multi-configuration workflow for windows and linux (I dont have any Apple hardware... CI said we are good). My development flow is as follows:

  • Writing cross platform code on windows using visual studio 20xx and multiple configurations. I first generate my project by loading all conan dependencies (debug and release) and then running premake to generate a multi configuration solution (only x64 currently). I develop my full code in VS and only go back to my pipeline when I need to adjust my dependencies or project setup.
  • When I have a release candidate (or since a few month also automatically with CI on every commit / PR) I do builds and tests on linux (CI also on Windows)

The whole generate has been written in a way so that it is compatible with this kind of multi configuration workflows. The observed behavior in #17345 expected behavior because the case was never covered by the efforts back then. However I see the point that an automatically detection is beneficial.

In case you want to see a (maybe a bit over engineered) version of my workflow, I have a project template that I use for all my (personal and youtube) projects as well as projects that I developed at work (previouse and current employer - thus production code): https://github.com/Moxibyte/MoxPP

A short comment on the bug:
For me this is not a bug! This is intended behavior. The generator generates a proper call section for all configuration that are available. Yes it is fenced into filters but this is how premake works. For me the old premake generator was not usable out of the box because it did not fence properly and thus was not multi configuration ready. When initially porting to v2.x we discussed that premake should support multiple configurations (#13275 (comment)) while trying to stay compatible with old projects. Breaking in some cases was considered as ok. I never have encountered a premake5.lua file without unset architecture (I don't even know what premake does... it probably default to 32-Bit...) and/or configurations and thus @Enhex issue/usage was never in the scope of the port.
In your case @Enhex you have already set the correct configurations in your premake5.lua file. The only thing you miss is the architecture call in your workspace config. In case you want to support 32Bit and 64Bit take a look at premakes platforms.
If you fix this and run conan twice (one time getting debug and one time getting release) you get a working project for both configurations.
(Yes I know this should be written into the issue not this pull... I will write a short version of this comment in the issue soon afterwards)

Now some comments on the suggested changes:
I really like the idea that it shall be possible to automatically detect the configuration when there is only configuration in the project.

However with the suggested changes I have some concerns. All of them are more philosophically concerns than actual technical issues:

  • Breaking changes: even if this is only a minor one, changing the name to PremakeMultiDeps is breaking all my projects and the projects of the companies i have worked for / currently working for. I see that the fix for the projected is fast and easy however renaming the "more capable" configuration and replacing it with a simplified version would for me not be the way to go.
  • Having two versions in general: For me we should rather aim in writing a universal generator that works for both intends than having two versions and require the user to decide on one (especially since the premake generator is not documented). For me we should adapt the implementation of the current generator so that it also fulfills the need of non multi configuration projects.
  • Generating premake code: The suggested changes call the premake configurations and architecture functions in a generated file. This is for me a no go. A package manager (for my understandings) should not be responsible of setting how the project builds/works (At least in the non toolchain version). For me the supported architecture and configuration(s) should be the job of the build system. If you set thees options correctly in your project the current premake generator works as intended. You can even go as far as setting all the options your project supports, the generator will then supply conan building and linking for all configuration it has builds for.

So I personally would not recommend merging this PR!

What I would recommend rather than merging this PR:
I for sure see the point of having conan dictate the way your project is build (especially when it come to the architecture). However as I have already written I would rather suggest modifying the current generator rather than merging the proposed changes.
It would be really easy to add a small reflection api to the conandeps.premake5.lua file either by reflecting the conandeps array at premake5 runtime or by generating a conaninfo array at conan runtime (a bit more work but would be my favorite).

Suggested API / conaninfo-Array:

conaninfo = {}
conaninfo["configurations"] = {}
conaninfo["configurations"]["release_x86_64"] = { "release", "x86_64" }
conaninfo["configurations"]["debug_x86_64"] = { "debug", "x86_64" } -- Would be generate for multiple configurations
-- Maybe even more information about the conan environment... List of libs... etc...

Suggested API / Public premake-conan API:

-- All of the following functions would print a YELLOW warning in case the array has more than one element

conaninfo_get_current()       -- Return the key of the first array element ("release_x86_64")
conaninfo_get_configuration() -- Returns the fist value of the arrays first element ("release")
conaninfo_get_architecture()  -- Returns the second value of the arrays first element ("x86_64")

This would allow you (@Enhex) to adapt your preamek5.lua file like this:

-- ...
workspace("nanovg")
    location(location_dir) 
    architecture(conaninfo_get_architecture())
    configurations { conaninfo_get_configuration() }

    project("nanovg"):
        -- ...
        conan_setup()
        -- ...   

This would also empower more advanced usage.

Conclusion:
Maybe @memsharded can you give us some insight of what you think? I would have time the next 3 days to implement my proposed changes. In case my changes shall be implemented please also give me a heads up on how we should handle the test coverage on the two use-cases.
This also reminded me that I still have the pending full Toolchain in progress. Maybe I have time thees days to also continue working on this in order to finally "finish" the full premake integration.

@Ohjurot
Copy link
Contributor

Ohjurot commented Nov 21, 2024

@memsharded I have see you already did some work on the toolchain in your recent PR #17350 very nice :D Thank you!

Is it still desired that I take on the work on this generator to abstract away all the OS dependent if else logic from the "users" recipes into the generator?

This was some draft that I was working on some month ago:
develop2...Ohjurot:conan:feature/premake_r3

@memsharded
Copy link
Member

Breaking changes: even if this is only a minor one, changing the name to PremakeMultiDeps is breaking all my projects and the projects of the companies i have worked for / currently working for. I see that the fix for the projected is fast and easy however renaming the "more capable" configuration and replacing it with a simplified version would for me not be the way to go.
Having two versions in general: For me we should rather aim in writing a universal generator that works for both intends than having two versions and require the user to decide on one (especially since the premake generator is not documented). For me we should adapt the implementation of the current generator so that it also fulfills the need of non multi configuration projects.

yes, I also agree with these points, and I think that this PR shouldn't be merged.

I also agree that I see some potential for improvements, as long as they are compatible and not breaking, with confs/opt-ins

I have see you already did some work on the toolchain in your recen

Yes, not really the toolchain, but the Premake cli wrapper. PremakeToolchain would be a separate effort, but at least the existing cli wrapper was broken, so added a full functional test, only pending of adding Premake to CI.

@Enhex
Copy link
Contributor Author

Enhex commented Nov 22, 2024

I never have encountered a premake5.lua file without unset architecture (I don't even know what premake does... it probably default to 32-Bit...)

Premake's basic tutorial doesn't use it, so probably a lot of Premake users don't either (generally it isn't needed unless you're cross compiling or targeting 32 bit from 64 bit):
https://premake.github.io/docs/Your-First-Script
also when you call compilers directly you don't need to explicitly specify the architecture so i assume Premake defaults to not telling the compiler the arch and letting it do its default.

it doesn't make sense for a user to have to manually specify architecture in both Conan and Premake because it's wrong if they mismatch, thus Conan should tell Premake what architecture to use.
unless you want to do multi-architecture in a single build folder too.
similar to what you suggested perhaps it's better to have Conan generate a collection of used configuration options (build type and architecture) across several conan calls and aggregate them, possibly by generating them into separate files in a folder and merging their contents for the premake5.lua to consume, which completely automates the whole multi config stuff on the Premake side.

Breaking changes: even if this is only a minor one, changing the name to PremakeMultiDeps is breaking all my projects

i was following the Conan 1 naming (example: cmake, cmake_multi, visual_studio, visual_studio_multi).
I have no problem keeping it the same and using something like PremakeSingleDeps instead.

Generating premake code: The suggested changes call the premake configurations and architecture functions in a generated file. This is for me a no go. A package manager (for my understandings) should not be responsible of setting how the project builds/works

but users do tell Conan what arch and build type to use.
ideally the build script automatically uses the settings Conan used for its dependencies so there are no mismatches and no need for manual work.

this PR's generator works well for me and for the sake of saving time I currently use it via my fork, and plan to use it as a custom generator in the future.
since it doesn't fit Conan 2's requirement I'm closing the PR, though I think for the sake of other Premake users it should be made simpler to use or at least document the requirements for the premake5.lua file because I believe a regular Premake user will face the same problems I did.

@Enhex Enhex closed this Nov 22, 2024
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.

4 participants