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

[lua] Fix library type and usage #24436

Merged
merged 13 commits into from
May 13, 2022
Merged

Conversation

JackBoosY
Copy link
Contributor

@JackBoosY JackBoosY commented Apr 27, 2022

Since the library generated by lua's feature cpp uses the same source as the c version of the lua library, using SET_SOURCE_FILES_PROPERTIES(PROPERTIES LANGUAGE CXX) anywhere in cmake will cause the c library to also be built as the cpp version.
So I made a copy of the source code and just declared them as CXX to fix this.
Also export the INTERFACE_INCLUDE_DIRECTORIES for usage.

Fixes #24413

Already tested build rmlui with / without lua cpp feature.

@JackBoosY JackBoosY added category:port-bug The issue is with a library, which is something the port should already support info:internal This PR or Issue was filed by the vcpkg team. labels Apr 27, 2022
@JackBoosY JackBoosY mentioned this pull request Apr 27, 2022
ports/lua/CMakeLists.txt Outdated Show resolved Hide resolved
ports/lua/CMakeLists.txt Outdated Show resolved Hide resolved
@PhoebeHui PhoebeHui marked this pull request as draft May 5, 2022 02:25
@JackBoosY JackBoosY marked this pull request as ready for review May 6, 2022 06:56
@JackBoosY JackBoosY requested a review from LilyWangLL May 6, 2022 07:04
@LilyWangLL LilyWangLL added the info:reviewed Pull Request changes follow basic guidelines label May 6, 2022
ports/lua/CMakeLists.txt Outdated Show resolved Hide resolved
@BillyONeal
Copy link
Member

Sorry, I should have clarified: the thing I want us to try is to have different directories with CMakeLists.txts in them, without any copying of source files. (If I have time today I'm going to try this...)

@JackBoosY
Copy link
Contributor Author

According to the official doc:

New in version 3.18: By default, source file properties are only visible to targets added in the same directory (CMakeLists.txt). Visibility can be set in other directory scopes using one or both of the following options:

So we can make a new CMakeLists.txt, add the cpp code to it to avoid this.

@JackBoosY JackBoosY requested a review from BillyONeal May 12, 2022 08:29
Copy link
Contributor

@dg0yt dg0yt left a comment

Choose a reason for hiding this comment

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

  • IMO the same list variables should be used for main directory and for C++ subdirectory. Avoid redundancy. Of course it needs absolute paths.
  • Did you consider moving to file(GLOB ...) to collect the headers and sources?
  • I see the install commands using absolute destination paths. In general, the preferred form is relative paths, and it is shorter. (Not so relevant for vcpkg but enough to catch my eye.)

Copy link
Member

@BillyONeal BillyONeal left a comment

Choose a reason for hiding this comment

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

This addresses my concern of multiple copies of sources confusing debuggers, thanks!

I would like to see @dg0yt's comments addressed though.

ports/lua/portfile.cmake Outdated Show resolved Hide resolved
@JackBoosY
Copy link
Contributor Author

  • IMO the same list variables should be used for main directory and for C++ subdirectory. Avoid redundancy. Of course it needs absolute paths.

Such as the headers and sources?

  • Did you consider moving to file(GLOB ...) to collect the headers and sources?

No, lua provides more files than listed. We should only choose some of them.

  • I see the install commands using absolute destination paths. In general, the preferred form is relative paths, and it is shorter. (Not so relevant for vcpkg but enough to catch my eye.)

I will make some update for them.

* Use "supports" on features rather than if tests plus message FATAL_ERROR
* Deduplicate ENABLE_LUA_CPP and COMPILE_AS_CPP
* Add quotes.
* Use file(INSTALL rather than configure_file(COPYONLY)
@BillyONeal BillyONeal merged commit 32d25e6 into microsoft:master May 13, 2022
@BillyONeal
Copy link
Member

Thanks for your fixes!

@JackBoosY JackBoosY deleted the dev/jack/24413 branch May 16, 2022 02:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:port-bug The issue is with a library, which is something the port should already support info:internal This PR or Issue was filed by the vcpkg team. info:reviewed Pull Request changes follow basic guidelines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

rmlui build failure
4 participants