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

CMakeLists: Change DiscIO dependency from common to core #10372

Merged
merged 1 commit into from Jan 16, 2022

Conversation

Pokechu22
Copy link
Contributor

DiscIO depends on some IOS functions and other functions, which are in Core and not Common. This results in link errors if using DiscIO on its own (which is why DolphinTool had a listed dependency on videocommon; videocommon has a dependency on core so adding that made things build).

DiscIO depends on some IOS functions and other functions, which are in Core and not Common.  This results in link errors if using DiscIO on its own (which is why DolphinTool had a listed dependency on videocommon; videocommon has a dependency on core so adding that made things build).
Copy link
Member

@JosJuice JosJuice left a comment

Choose a reason for hiding this comment

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

That DiscIO depends on Core isn't great... But that's not this PR's fault, so let's not let that hold this PR up.

@@ -66,7 +66,7 @@ add_library(discio

target_link_libraries(discio
PUBLIC
common
core
Copy link
Member

Choose a reason for hiding this comment

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

Why not keep the common line?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

core depends on common already, so it's redundant. But there doesn't seem to be any problem with keeping it either.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I just figured there could be a point in avoiding implicit dependencies. But I don't know what our typical stance is on this in CMake.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

DolphinQt only specifies core and not common so I think implicit/transitive dependencies are fine.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, seems consistent with the other targets and core will always depend on common. That said, I think our policy should be to avoid transitive dependencies in general (e.g. if both core and discio use fmt then they should both depend on the fmt target explicitly -- discio should not be relying on core to depend on fmt::fmt). core/common would be an exception to the rule.

@dolphin-emu-bot
Copy link
Contributor

FifoCI detected that this change impacts graphical rendering. Here are the behavior differences detected by the system:

  • burnout2-vehicletextures on mvk-osx-m1: diff
  • DKCR-Char on mvk-osx-m1: diff
  • DKCR-fast-depth on mvk-osx-m1: diff
  • ea-pink on mvk-osx-m1: diff
  • inverted-depth-range on mvk-osx-m1: diff
  • rs2-glass on mvk-osx-m1: diff
  • rs3-bumpmapping on mvk-osx-m1: diff

automated-fifoci-reporter

Copy link
Member

@JosJuice JosJuice left a comment

Choose a reason for hiding this comment

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

Sounds fine then.

@leoetlino leoetlino merged commit cb19472 into dolphin-emu:master Jan 16, 2022
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
4 participants