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

Tools reorg #1498

Merged
merged 8 commits into from
Aug 7, 2024
Merged

Tools reorg #1498

merged 8 commits into from
Aug 7, 2024

Conversation

dpogue
Copy link
Member

@dpogue dpogue commented Oct 4, 2023

This is open for discussion, but seemed worthwhile to get it working first so people could take a look.

  • Moves all the non-client tools out of Sources/Plasma/Apps and into Sources/Tools:

    • plFileEncrypt
    • plFileSecure
    • plPageInfo
    • plPageOptimizer
    • plPythonPack
  • The exception is SoundDecompress which moved from Sources/Tools into Sources/Plasma/Apps (because it is sorta intended to be run by end users and distributed with the client)

  • Move all the Max Plugin stuff out of Sources/Tools into its own Sources/MaxPlugin folder so that it is more self-contained:

    • MaxComponent
    • MaxConvert
    • MaxExport
    • MaxMain
    • MaxPlasmaLights
    • MaxPlasmaMtls
  • Only build plLocalizationBenchmark when PLASMA_BUILD_TOOLS is on (arguably it's not a tool, but it was previously being built unconditionally)

  • Consolidate building of plClient for multiple platforms

Copy link
Contributor

@dgelessus dgelessus left a comment

Choose a reason for hiding this comment

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

This is a good reorganization IMO - the reasoning all makes sense to me. The previous split of the tools between Plasma/Apps and Tools always seemed pretty random. The new directory structure also aligns better with the PLASMA_BUILD_* CMake options, which I think is a good change.

CMakeLists.txt Outdated Show resolved Hide resolved
add_subdirectory(plFileEncrypt)
add_subdirectory(plPageInfo)
add_subdirectory(plPageOptimizer)
add_subdirectory(SoundDecompress)
Copy link
Contributor

Choose a reason for hiding this comment

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

This is now the only thing outside Sources/Tools that's conditional on PLASMA_BUILD_TOOLS - maybe it should use a different option? I don't really have any better ideas though. We probably don't want to build it unconditionally, but does it need its own PLASMA_BUILD_SOUNDDECOMPRESS option? I guess we could group it under PLASMA_BUILD_LAUNCHER..?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm definitely open to suggestions here, but don't really have any better ideas myself 😕

@dpogue dpogue force-pushed the tools-reorg branch 2 times, most recently from e123a06 to 60fb605 Compare October 8, 2023 04:00
@dpogue dpogue marked this pull request as ready for review October 8, 2023 04:02
@Hoikas
Copy link
Member

Hoikas commented Oct 15, 2023

These commits should probably be added to .git-blame-ignore-revs

@dpogue
Copy link
Member Author

dpogue commented Oct 16, 2023

These commits should probably be added to .git-blame-ignore-revs

I can do that, but I'm unclear on whether that's necessary in the case of git mv to move files around. The file contents didn't change, and keeping knowledge of the path change seems necessary to handle blaming properly against older versions?

Copy link
Member

@Hoikas Hoikas left a comment

Choose a reason for hiding this comment

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

I don't think there are any objections here, so we should probably take that as approval. Let's get this done before the tools change again, causing conflicts.

@dpogue dpogue merged commit 1159c5e into H-uru:master Aug 7, 2024
17 checks passed
@dpogue dpogue deleted the tools-reorg branch August 7, 2024 04:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants