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

Externals: Exclude libcurl.rc from the build #11394

Merged
merged 1 commit into from Jan 7, 2023

Conversation

Pokechu22
Copy link
Contributor

For some reason, when this is included, the linking step creates a temporary file in %TEMP% with a random name; the file is deleted afterwards and a new random name is used on a later build. Because this file doesn't exist on a later build, curl gets re-linked each time, and then all of the projects that depend on curl also get re-linked. This adds around 10 seconds to the build time even for small changes.

To make things worse, I don't think libcurl.rc does anything useful since we statically link curl; I believe the metadata contained in it only applies when building a dll. (It does seem to be included in curl.lib, but gets discarded when linking Dolphin.exe.)

See Build\x64\Release\curl\curl.tlog\Lib-link-cvtres.write.1.tlog for the log that shows this path (the file is also mentioned after setting Tools -> Options... -> Projects and Solutions -> Build and Run -> MSBuild project build output verbosity to diagnostic, but not in a useful way).

@shuffle2
Copy link
Contributor

shuffle2 commented Jan 6, 2023

To make things worse, I don't think libcurl.rc does anything useful since we statically link curl; I believe the metadata contained in it only applies when building a dll. (It does seem to be included in curl.lib, but gets discarded when linking Dolphin.exe.)

yes that's correct (it's used in the target binary image, which could also be .exe or something).

@@ -358,7 +358,9 @@
<ClInclude Include="lib\x509asn1.h" />
</ItemGroup>
<ItemGroup>
<ResourceCompile Include="lib\libcurl.rc" />
<ResourceCompile Include="lib\libcurl.rc">
Copy link
Contributor

Choose a reason for hiding this comment

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

could also just delete the ItemGroup

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed, that should also work. There are a few other files that are also marked as excluded, though, which were added in 3b3551f (by you). Any reason why that was done, instead of deleting the relevant lines?

Copy link
Contributor

Choose a reason for hiding this comment

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

Since those are 'normal' compiled items files, I thought it might help in the future to make updating the External easier, so you could tell which files were purposefully excluded vs. new files after updating. In retrospect I don't think it's really that helpful. I agree I haven't been uniform in that over time.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll just delete the item group instead, then.

For some reason, when this is included, the linking step creates a temporary file in %TEMP% with a random name; the file is deleted afterwards and a new random name is used on a later build. Because this file doesn't exist on a later build, curl gets re-linked each time, and then all of the projects that depend on curl also get re-linked. This adds around 10 seconds to the build time even for small changes.

To make things worse, I don't think libcurl.rc does anything useful since we statically link curl; I believe the metadata contained in it only applies when building a dll. (It does seem to be included in curl.lib, but gets discarded when linking Dolphin.exe.)

See Build\x64\Release\curl\curl.tlog\Lib-link-cvtres.write.1.tlog for the log that shows this path (the file is also mentioned after setting Tools -> Options... -> Projects and Solutions -> Build and Run -> MSBuild project build output verbosity to diagnostic, but not in a useful way).
@AdmiralCurtiss AdmiralCurtiss merged commit 7444f90 into dolphin-emu:master Jan 7, 2023
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
3 participants