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

Xmake Improvements #494

Merged
merged 9 commits into from
May 17, 2024
Merged

Xmake Improvements #494

merged 9 commits into from
May 17, 2024

Conversation

bitonality
Copy link
Contributor

@bitonality bitonality commented May 9, 2024

This pull request attempts to:

  1. Improve the inline documentation of our xmake system.
  2. Increase performance of xmake configuration code paths.
  3. Improve compliance with current xmake best practices.
  4. Allow for easier and more resilient code paths when sub-moduling UE4SS in other xmake projects.

Major improvements include:

Leveraging xmake rules to allow for modular on_config, on_load, etc overrides.

Instead of having a globally defined on_config() function that automatically gets applied to all targets, we should use xmake rules. Rules have their own on_xxx() functions that automatically get applied to any targets that have the rule assigned to them. This gives us finer control of which parts of the build system are responsible for applying any settings. The current setup of a global on_config() is that each target can only have ONE on_config(). This means that any targets that need special logic in their on_xxx() functions have to duplicate code to call the common on_xxx() and then run their own logic. This leads to code duplication and increases the chance of developer error since each overridden on_xxx() lives in the target's xmake.lua file which makes tracking down these errors more difficult. Using rules to define the on_xxx() behavior allows us to define the applying of settings in a single location.

Here's a code based example to supplement the prior description:

-- animal_example.lua

-- This global on_config is applied to each target.
-- Add -Danimal to both the "dog" and "cat" targets
on_config(function(target)
    target:add("defines", "animal")
end)

target("dog")

target("cat")

But what if we want to add a "dog" define ONLY to the dog target?

on_config(function(target)
    target:add("defines", "animal")
end)

target("dog")
    on_config(function(target)
        -- We have to add the "animal" define since this on_config overrides the global on_config.
        target:add("defines", {"dog", "animal"})
    end)

target("cat")

Leveraging rules simplifies this whole process

rule("animal_rule")
    on_config(function(target)
       target:add("defines", "animal")
    end)
    
-- Globally apply the `animal_rule` to all targets
-- We could also individually apply the rule by calling `add_rules("animal_rule")` in the target description scope
add_rules("animal_rule")

target("dog")
    on_config(function(target)
        -- This on_config does not override the behavior defined in rule("animal_rule").
        -- Both this on_config() and the `animal_rule` on_config() will run when this target is configured (yay!)
        target:add("defines", "dog")
    end)

target("cat")

Decrease the amount of mode parsing in the description scope

Previously, every call to is_mode_debug() and get_mode_runtimes() required a string.gmatch() to determine if the unreal mode (Game__Shipping__Win64, etc.) had "Debug" or "Dev" as the middle part of the triple. This ends up being called a lot since we had to call is_mode_debug() and get_mode_runtimes() every time we require() a package to determine which flavor of the package to use (debug/release + MD/MDd). I've opted to leverage set_runtimes(get_mode_runtimes()) in the global scope which does the following:

Applies the correct runtimes to every target - Removes the need to

-- This codepath gets hit for every target
on_config(function(target) 
    target:set("runtimes", get_mode_runtimes()) 
end)

Applies the correct runtimes to every package.

-- add_requires will automatically inherit the global runtime so we don't need to specify
add_requires("raw_pdb", { debug = is_mode_debug(), configs = { runtimes = get_mode_runtimes() } })

-- We can instead...
add_requires("raw_pdb", { debug = is_mode_debug() })

Add a directory for our custom xmake modules

Instead of having to store a ue4ssScriptsRoot or similar variable, I've opted to provide a directory to store any code that we want to import() into the lua script context.

-- Instead of
import("target_helpers", { rootdir = get_config("scriptsRoot") })
-- We can now
import("target_helpers")

-- If we want to add more dirs to the modules path then we can just add_moduledirs() in the global scope

-- Any lua modules in this directory can be imported in the script scope by using
-- /modules/my_module.lua           import("my_module")
-- /modules/rules/my_module.lua     import("rules.my_module")
add_moduledirs("tools/xmakescripts/modules")

This also helps enforce scope segregation. Code defined in a module is intended to be used in the script scope (ex. on_config(), on_load(), after_xxx(), etc. It is intentionally impossible to import() in the description scope, so this helps make the intent more clear and promotes good code reuse.

Use rules instead of custom metadata values for the UE4SS Dependency logic

Previously, any ue4ss dependencies had a special set_values("ue4ssDep", true) in their target definition. This is used to indicate that the target needs to add -D defines with RC_TARGET_EXPORTS and RC_TARGET_BUILD_STATIC. I've opted to create rules for these instead of setting an arbitrary ue4ssDep flag to indicate if we need to add the aforementioned defines.

Streamline UE4SS installation

We can now use xmake install --installdir=xxx to copy the UE4SS.dll, UE4SS.pdb, and dwmapi.dll to the --installdir. Nothing groundbreaking, but a nice quality of life addition.

@UE4SS
Copy link
Collaborator

UE4SS commented May 9, 2024

Does this PR need to update the docs for creating a C++ mod ?

@UE4SS UE4SS requested a review from localcc May 9, 2024 06:29
@Buckminsterfullerene02
Copy link
Member

Does this eventually link into the work you were doing for linux port? If so, some communication on where you are at for that would be nice. If not, improvements are nice and all, but there is not really much point in it if linux port remains stuck, as it being stuck for too long may warrant the switch back to cmake and try to fix the issues we had with it from that end instead, and your work here will be limited to an educational time expense.

@bitonality
Copy link
Contributor Author

Does this eventually link into the work you were doing for linux port? If so, some communication on where you are at for that would be nice. If not, improvements are nice and all, but there is not really much point in it if linux port remains stuck, as it being stuck for too long may warrant the switch back to cmake and try to fix the issues we had with it from that end instead, and your work here will be limited to an educational time expense.

I am unclear on what the current blockers for the linux port are. I provided the workaround for triggering patternsleuth rebuild
xmake require --force --extra="{configs={cargo_toml='~/LinuxUE4SS/deps/first/patternsleuth_bind/Cargo.toml'}}" cargo::patternsleuth_bind

I also indicated what flag changes needed to be made for compilation without any manual setting of flags from the cli

        ["cxflags"] = {
            "clang::-fno-delete-null-pointer-checks",
            "clang::-stdlib=libc++",
            "clang::-fexperimental-library"
        },
        ["ldflags"] = {
            "-stdlib=libc++",
            "-fuse-ld=lld"
        }

If the linux contributors are opposed to using the workarounds for the time being then I'll submit a PR which includes the automatic patternsleuth rebuild without the aforementioned workaround. The rust behavior is the same on windows and linux so that work needs to be done anyways. That rust work is based on this PR, but I suppose I can extrapolate that work and submit a separate PR against main branch with the rust improvements.

@UE4SS
Copy link
Collaborator

UE4SS commented May 10, 2024

Does this eventually link into the work you were doing for linux port? If so, some communication on where you are at for that would be nice. If not, improvements are nice and all, but there is not really much point in it if linux port remains stuck, as it being stuck for too long may warrant the switch back to cmake and try to fix the issues we had with it from that end instead, and your work here will be limited to an educational time expense.

I am unclear on what the current blockers for the linux port are. I provided the workaround for triggering patternsleuth rebuild xmake require --force --extra="{configs={cargo_toml='~/LinuxUE4SS/deps/first/patternsleuth_bind/Cargo.toml'}}" cargo::patternsleuth_bind

I also indicated what flag changes needed to be made for compilation without any manual setting of flags from the cli

        ["cxflags"] = {
            "clang::-fno-delete-null-pointer-checks",
            "clang::-stdlib=libc++",
            "clang::-fexperimental-library"
        },
        ["ldflags"] = {
            "-stdlib=libc++",
            "-fuse-ld=lld"
        }

If the linux contributors are opposed to using the workarounds for the time being then I'll submit a PR which includes the automatic patternsleuth rebuild without the aforementioned workaround. The rust behavior is the same on windows and linux so that work needs to be done anyways. That rust work is based on this PR, but I suppose I can extrapolate that work and submit a separate PR against main branch with the rust improvements.

I don't know if there's a consensus surrounding which one of your solutions is preferred, but I made my opinion known on that.

This doesn't seem like much of a blocker regardless because if I recall correctly, without solving this, at most it will require some extra effort when you make changes to PS or external dependencies.
We should be able to easily change this at any point.

As far as compile and link flags are concerned, I don't know if the they've been implemented in the linux PR but this also shouldn't be a blocker since it's a solved problem as far as I know.

Waiting on @Buckminsterfullerene02 to clarify if there's anything else blocking the linux PR when it comes to the build system.

@Buckminsterfullerene02
Copy link
Member

I thought there was still a blockage since nothing had been done on it in terms of actually implementing one of the solutions bitonality provided.

I read that he was working on something with xmake which I thought was the Linux port implementing a preferred solution, but now I know it's this PR, so that clears up that miscommunication.

In terms of Linux port, who is going to take responsibility for implementing whichever required solution? Currently it feels like everyone is assuming someone else is doing it when in fact it seems no one has. Perhaps @localcc ? Or are we waiting on other maintainers to provide their thoughts on their preferred solution?

@localcc
Copy link
Contributor

localcc commented May 11, 2024

I am happy to work on the linux port xmake, just assumed that someone else was already doing it, I'll take it then

deps/third/xmake.lua Outdated Show resolved Hide resolved
@UE4SS
Copy link
Collaborator

UE4SS commented May 11, 2024

In terms of Linux port, who is going to take responsibility for implementing whichever required solution? Currently it feels like everyone is assuming someone else is doing it when in fact it seems no one has.

I am happy to work on the linux port xmake, just assumed that someone else was already doing it, I'll take it then

As it is @Yangff that created the PR, I've pretty much been waiting for them to implement one of the solutions from @bitonality, but I'm sure they won't mind if @localcc gets it up and running so that they can get unblocked for the rest of the PR.

As far as which solution, I can't tell you that but I can tell you that if I were taking up charge of the PR, I'd go with my stated preference because no one suggested anything else nor objected to it and you can only wait so long before continuing before it becomes entirely unreasonable to wait any longer.
Obviously this is barring any problems one might run into while implementing the solution.

@UE4SS
Copy link
Collaborator

UE4SS commented May 11, 2024

Also I'd like to point out that the last release was on Feb 14 and I don't believe the Linux port will be ready at any point soon, but please correct me if I'm wrong.
If we don't at least create some kind of cut off date for new features soon (including Linux support) we'll risk entering into another infinite development loop.

@localcc
Copy link
Contributor

localcc commented May 11, 2024

imo we should do a 3.1 release with TSet support and linux can come later, we can't delay for much longer so we don't get another 3.0 incident.

@UE4SS
Copy link
Collaborator

UE4SS commented May 11, 2024

@localcc Did you mean to approve this PR even though your questions (that seem important, especially the second one) has yet to be answered, and are currently unresolved ?

@UE4SS
Copy link
Collaborator

UE4SS commented May 11, 2024

imo we should do a 3.1 release with TSet support and linux can come later, we can't delay for much longer so we don't get another 3.0 incident.

I agree with this.
In fact, I'd say a release should be done as soon as whoever handles that these days has the time and willpower to do it, unless I'm forgetting something or anyone disagrees ?

Who is in charge of making releases these days anyway ?
I'd consider doing much more frequent releases myself but I can't do that as long as internal conversations keep taking place on discord, and I've also heard that a checklist exists on the discord server which obviously I'd need as well.
I also strongly dislike that we don't create a branch for each release because it means it's a lot harder to back-port hotfixes after breaking changes have been made to main, and this is something I'd like to change in the future.

@Yangff
Copy link
Contributor

Yangff commented May 11, 2024

I've pretty much been waiting for them to implement one of the solutions from @bitonality,

hmm.. I was told that someone else is solving the rust issue with xmake.. and it would be nice if @localcc can solve that . After all, I'm not familiar with how xmake works internally.. Also, imgui (with graphical one) seems to work fine in their locales, although I can't get it to compile correctly..

@Yangff
Copy link
Contributor

Yangff commented May 11, 2024

I think for ci it should build without problem because it will be clean build anyway (correct me ?), and it's more about the development experience I think..

@Buckminsterfullerene02
Copy link
Member

imo we should do a 3.1 release with TSet support and linux can come later, we can't delay for much longer so we don't get another 3.0 incident.

I agree with this. In fact, I'd say a release should be done as soon as whoever handles that these days has the time and willpower to do it, unless I'm forgetting something or anyone disagrees ?

Who is in charge of making releases these days anyway ? I'd consider doing much more frequent releases myself but I can't do that as long as internal conversations keep taking place on discord, and I've also heard that a checklist exists on the discord server which obviously I'd need as well. I also strongly dislike that we don't create a branch for each release because it means it's a lot harder to back-port hotfixes after breaking changes have been made to main, and this is something I'd like to change in the future.

I agree that we don't want another 3.0, however there are some good reasons why I have not considered making a new release yet:

build system
xmake is still a pending system, and has been since February. It would be very bad to make a new release with this build system when linux support is still pending and we don't know if xmake will actually work well on linux. If it turns out we have to rollback to cmake because of the linux xmake issues, then having a new release when all C++ developers are required to recompile their mods on xmake and having to change back to cmake after would be a catastrophic failure of management. xmake must be either fully implemented or not for the next release.

C++ mod versioning
Aside from build system, ue4ss has a very gaping flaw: every update which changes ABI (i.e. every one) requires C++ mod developers to recompile their mods against the latest update AND all users must be using the latest version to run that version. This is a problem now that ue4ss is being used by tens or hundreds of thousands of users.

If C++ developers update their mods to work on latest ue4ss version, it will not for the older versions, which most users will still be using. If some developers update their mods for latest and some don't, users will be forced to choose the version they want to stay on and thus have to lose access to the mod (or mod updates for older ones on new version) that is not on the version they are on.

The issue is worsened due to the fact that mod managers don't really provide the same flexibility as manual install - that is, when a mod update is pushed, all users will recieve that update, and likely including the new dependency on new ue4ss version, which breaks all the other C++ mods not updated yet. Users have to manually downgrade to an older version of the C++ mod and ue4ss (if they understand this, which most users won't unless being explicitly told) until most/all of the C++ mod developers have updated their mods for the newest version of ue4ss.

All this is to say, that frequent ue4ss updates that change ABI are no longer a luxury. We need to have a think about what to do before releasing updates, and updates should ideally be tested heavily before release (unlike 3.0 was, we had some last minute changes that were not tested well and thus caused bugs that were not acceptable to have in a major release; I take full responsibility for that) - as hotfixes (especially if spaced too far apart) will excarberate the problem.

data table support
The most requested feature by far is data table support for lua and C++ modding. This (and/or TSet/TMap support) would pretty much be the only reason for the majority of our mod developers to update for new version. This is more of a subjective one but the plan was always for next version to be released with data table support regardless of what else came with it, but then that got blocked by FName which got blocked by dynamic type libs which is blocked by not being implemented yet.

Apologies for the mini essay but hope this clears up roughly what is going on in my head at least.

@narknon
Copy link
Collaborator

narknon commented May 11, 2024

This is the wrong place for this conversation but I'm confused why it would be "3.1" when xmake is the most breaking change possible. There are other breaking changes as well.

@UE4SS
Copy link
Collaborator

UE4SS commented May 11, 2024

All this is to say, that frequent ue4ss updates that change ABI are no longer a luxury. We need to have a think about what to do before releasing updates, and updates should ideally be tested heavily before release (unlike 3.0 was, we had some last minute changes that were not tested well and thus caused bugs that were not acceptable to have in a major release; I take full responsibility for that) - as hotfixes (especially if spaced too far apart) will excarberate the problem.

Yes, much greater thought should be put into how we deal with releases.

First of all, hotfixes are not supposed to be breaking in any way at all, that was never the intent.
Someone running a future version 3.1.4 should be able to run mods made for 3.1.3, 3.1.2, 3.1.1 and 3.1.0.
This would require us to actually define what a major, minor and hotfix update is.
If we use semver, only a major update can be break compatibility, while both minor and hotfix has to be backwards compatible, meaning that someone running version 3.2.0 should be able to run mods made for 3.1.0.

I think having dedicated branches for every version would be very helpful, that way we can make pull requests and upon merging into main we can also decide to back-port it into the current non-breaking version when it's appropriate.
It would allow us to make whatever breaking changes we want for the next major version while still being able to support the current version while maintaining compatibility.
Unless someone else has a better idea ? Because only using tags doesn't seem very workable to me.

@UE4SS
Copy link
Collaborator

UE4SS commented May 11, 2024

This is the wrong place for this conversation but I'm confused why it would be "3.1" when xmake is the most breaking change possible. There are other breaking changes as well.

As far as end-users are concerned, the change to xmake theoretically doesn't have to be a breaking change but it's entirely possible (and probably likely) that the command lines that our xmake configs are generating are different to what our old cmake configs were generating which could cause the change to xmake to be breaking.
We'd need to check if the change to xmake actually breaks anything as far as compatibility is concerned, specifically for C++ mods.

As far as developers are concerned, I don't know if bumping the version number is appropriate for a build system change, because the version number is primarily meant to be used by end-users and mod developers to understand what versions of UE4SS their mods are compatible with.
There might be a better solution for getting developers ready for a build system change, like providing a warning that such a change is happening and what they need to do to prepare and apply whatever changes that needs to be applied.

@Buckminsterfullerene02
Copy link
Member

Continued in #498 let's keep the rest of discussions here for just this PR.

@localcc
Copy link
Contributor

localcc commented May 11, 2024

@localcc Did you mean to approve this PR even though your questions (that seem important, especially the second one) has yet to be answered, and are currently unresolved ?

Yeah the questions were clarified in DMs and should be clarified here as well, but the general state of the PR is green.

@localcc
Copy link
Contributor

localcc commented May 11, 2024

xmake is supported by linux and works fine there, the issues we are having are caused by the lack of experience with it currently, but that's nothing that can't be fixed and definitely nothing that would warrant a switch back.

data tables are pending and this requires the DynamicType library integration which I am slowly working on.

@Buckminsterfullerene02
Copy link
Member

@localcc please continue the conversation in #498...

@Buckminsterfullerene02
Copy link
Member

Buckminsterfullerene02 commented May 12, 2024

@bitonality C++ mod docs needs updating - mod xmake.lua is wrong

@Buckminsterfullerene02
Copy link
Member

Everything seems to work good!

xmake.lua Show resolved Hide resolved
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.

6 participants