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

Introduction of the cargo.build rule to manage patternsleuth as a target #516

Merged
merged 6 commits into from
May 29, 2024

Conversation

bitonality
Copy link
Contributor

@bitonality bitonality commented May 17, 2024

# WARNING

This change is waiting on a fix to be released in xmake v2.9.2 xmake-io/xmake#5034

I'm downgrading this to a draft until users can run this script with a stable release of xmake.

If you want to test it, you can xmake update -s dev which will update your xmake version with the latest dev scripts.

PR NOTES

Continuation of previous discussions in which we decided to build patternsleuth as a cargo target.

#384
#494

This promotes patternsleuth to its own target that can be built with xmake build patternsleuth.

It also leverages cargo's incremental build and parses the cargo .d files for robust detection if rebuilds are needed or not. I also cache the release/debug dependency crates to allow for faster builds across modes on the same machines/CI machines.

xmake clean patternsleuth
xmake build patternsleuth
xmake build --rebuild patternsleuth
etc...

This also fixes somewhat of a glaring oversight in the cargo package pipeline involving msvcrt.lib and msvcrtd.lib. The cargo rule I wrote has the logic for detecting windows runtimes and correctly linking them.

This is also an improvement over the cargo package pipeline since I leveraged cargo to report precisely what libraries it expects to be linked with instead of us guessing upstream.

We now can remove

add_links("ws2_32", "advapi32", "userenv", "ntdll", "oleaut32", "bcrypt", "ole32", { public = true })

because cargo now directly reports what it needs for linking and we automatically add those flags at link time. This should help with cross compatibility since we won't have to generate a magical linking matrix for specific platforms/etc.

I'm also happy with the generic Cargo.toml solution I implemented because we can very easily add more pure-rust Cargo packages into the build system with very little additional work. The cargo specific logic has all been abstracted away in the rust_rules.lua file and people should only have to write vanilla xmake definitions.

deps/first/xmake.lua Outdated Show resolved Hide resolved
@Buckminsterfullerene02
Copy link
Member

@bitonality

[ 87%]: linking.Game__Shipping__Win64 ArgsParser.exe
error: cannot open file: Intermediates.deps\patternsleuth_bind\windows\x64\Game__Shipping__Win64\native-libs.txt, No such file or directory

@bitonality
Copy link
Contributor Author

@bitonality

[ 87%]: linking.Game__Shipping__Win64 ArgsParser.exe error: cannot open file: Intermediates.deps\patternsleuth_bind\windows\x64\Game__Shipping__Win64\native-libs.txt, No such file or directory

Did it look like your build hit the incremental cache at any point? If you try xmake build --rebuild do you get different results?

@Buckminsterfullerene02
Copy link
Member

Buckminsterfullerene02 commented May 19, 2024

@bitonality
[ 87%]: linking.Game__Shipping__Win64 ArgsParser.exe error: cannot open file: Intermediates.deps\patternsleuth_bind\windows\x64\Game__Shipping__Win64\native-libs.txt, No such file or directory

Did it look like your build hit the incremental cache at any point? If you try xmake build --rebuild do you get different results?

First time started at 62% when usually I think when it starts incremental build from like 50% ish?

[ 87%]: linking.Game__Shipping__Win64 ArgsParser.exe
[ 91%]: linking.Game__Shipping__Win64 patternsleuth_bind.lib
[ 91%]: archiving.Game__Shipping__Win64 LuaMadeSimple.lib
[ 91%]: archiving.Game__Shipping__Win64 DynamicOutput.lib
[ 91%]: archiving.Game__Shipping__Win64 ParserBase.lib
[ 91%]: linking.Game__Shipping__Win64 proxy_generator.exe
error: error: crate name `` passed to --extern is not a valid ASCII identifier

@bitonality
Copy link
Contributor Author

bitonality commented May 19, 2024

@bitonality
[ 87%]: linking.Game__Shipping__Win64 ArgsParser.exe error: cannot open file: Intermediates.deps\patternsleuth_bind\windows\x64\Game__Shipping__Win64\native-libs.txt, No such file or directory

Did it look like your build hit the incremental cache at any point? If you try xmake build --rebuild do you get different results?

First time started at 62% when usually I think when it starts incremental build from like 50% ish?

[ 87%]: linking.Game__Shipping__Win64 ArgsParser.exe [ 91%]: linking.Game__Shipping__Win64 patternsleuth_bind.lib [ 91%]: archiving.Game__Shipping__Win64 LuaMadeSimple.lib [ 91%]: archiving.Game__Shipping__Win64 DynamicOutput.lib [ 91%]: archiving.Game__Shipping__Win64 ParserBase.lib [ 91%]: linking.Game__Shipping__Win64 proxy_generator.exe error: error: crate name `` passed to --extern is not a valid ASCII identifier

I think I caught an edge case for people updating with existing cached config/incremental.

Could you pull latest and see if it works with just regular xmake build and if that has issues, can you try xmake build --rebuild

@Buckminsterfullerene02
Copy link
Member

I still get the same error (with --rebuild)

@bitonality
Copy link
Contributor Author

I still get the same error (with --rebuild)

How about xmake clean --all and then xmake build?

@Buckminsterfullerene02
Copy link
Member

I still get the same error (with --rebuild)

How about xmake clean --all and then xmake build?

Same error.

@bitonality
Copy link
Contributor Author

I still get the same error (with --rebuild)

How about xmake clean --all and then xmake build?

Same error.

Oh shoot. Hadn't realized that I change I made in xmake hasn't been shipped yet...
xmake-io/xmake#5034

If you want to test against the dev scripts, you can update your scripts only by running
xmake update -s dev

No worries if not. I guess I'll have to pump the brakes on this change until xmake v2.9.2 releases.

@bitonality bitonality marked this pull request as draft May 19, 2024 17:41
@UE4SS
Copy link
Collaborator

UE4SS commented May 20, 2024

I still get the same error (with --rebuild)

How about xmake clean --all and then xmake build?

Same error.

Oh shoot. Hadn't realized that I change I made in xmake hasn't been shipped yet... xmake-io/xmake#5034

If you want to test against the dev scripts, you can update your scripts only by running xmake update -s dev

No worries if not. I guess I'll have to pump the brakes on this change until xmake v2.9.2 releases.

I think you should edit the build requirements in this PR to state the required xmake version in that case.
I just noticed we don't currently mention which version is required, and clearly this change will require a particular version or greater and people need to know this.

@bitonality
Copy link
Contributor Author

I still get the same error (with --rebuild)

How about xmake clean --all and then xmake build?

Same error.

Oh shoot. Hadn't realized that I change I made in xmake hasn't been shipped yet... xmake-io/xmake#5034
If you want to test against the dev scripts, you can update your scripts only by running xmake update -s dev
No worries if not. I guess I'll have to pump the brakes on this change until xmake v2.9.2 releases.

I think you should edit the build requirements in this PR to state the required xmake version in that case. I just noticed we don't currently mention which version is required, and clearly this change will require a particular version or greater and people need to know this.

I plan on enforcing the minimum required with the project-wide set_xmakever(). I just need to wait for that version to exist :P

@bitonality
Copy link
Contributor Author

I still get the same error (with --rebuild)

How about xmake clean --all and then xmake build?

Same error.

Oh shoot. Hadn't realized that I change I made in xmake hasn't been shipped yet... xmake-io/xmake#5034
If you want to test against the dev scripts, you can update your scripts only by running xmake update -s dev
No worries if not. I guess I'll have to pump the brakes on this change until xmake v2.9.2 releases.

I think you should edit the build requirements in this PR to state the required xmake version in that case. I just noticed we don't currently mention which version is required, and clearly this change will require a particular version or greater and people need to know this.

I plan on enforcing the minimum required with the project-wide set_xmakever(). I just need to wait for that version to exist :P

image
Example output for what that looks like from a user pov

@bitonality bitonality marked this pull request as ready for review May 24, 2024 18:07
@bitonality
Copy link
Contributor Author

xmake v2.9.2 is now publicly available, so I'm resurrecting this PR with bolstered documentation.

@bitonality
Copy link
Contributor Author

The VS changes/documentation here likely resolves #526
Need a couple of more testers to confirm

@UE4SS
Copy link
Collaborator

UE4SS commented May 24, 2024

The VS changes/documentation here likely resolves #526
Need a couple of more testers to confirm

I tested xmake project -k vsxmake2022 -m "Game__Debug__Win64,Game__Shipping__Win64" -y, and configuration succeeded, and building both also succeeded.
This will definitely make the development experience less annoying for me, thanks.

UE4SS
UE4SS previously approved these changes May 27, 2024
Copy link
Collaborator

@UE4SS UE4SS left a comment

Choose a reason for hiding this comment

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

Accidentally approved, didn't realize the scope was bigger than I thought, I don't have the capacity to know if this PR is good enough to be merged.

@UE4SS UE4SS dismissed their stale review May 27, 2024 09:44

Accident

@bitonality
Copy link
Contributor Author

Accidentally approved, didn't realize the scope was bigger than I thought, I don't have the capacity to know if this PR is good enough to be merged.

I appreciate the caution. All new codepaths are behind the new --patternsleuth flag which is defaulted to be package (the current configuration in main), so the blast radius of potential breakage is small. That said, I have high confidence in this change and would like to start getting more executions on the new code path on an opt-in basis (particularly for the linux port).

@narknon narknon requested a review from localcc May 29, 2024 17:09
@narknon narknon removed the request for review from localcc May 29, 2024 17:20
README.md Show resolved Hide resolved
@narknon
Copy link
Collaborator

narknon commented May 29, 2024

@bitonality are your new commits intended to be on this PR?

@bitonality
Copy link
Contributor Author

@bitonality are your new commits intended to be on this PR?

Thanks for catching that - was testing CI on my fork and messed up the branch filters.

The unrelated commits are backed out and no longer part of this change

@narknon narknon merged commit 6c84261 into UE4SS-RE:main May 29, 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.

5 participants