Skip to content

Conversation

@giomba
Copy link
Collaborator

@giomba giomba commented Jun 12, 2025

I think that the title is self-explanatory.
I moved the set(Z80...) variable before the include of the library, because it is needed to build the docs.
This changes the previous behaviour (docs are always built, instead of only being built when building in debug mode, and the build output is very verbose), but this doesn't look like a big issues, since CMake is smart enough to only build it the first time.

@giomba giomba requested a review from giuliof June 12, 2025 21:35
@giomba giomba force-pushed the giomba/build-libs-docs-in-container branch from 0240dae to c77311f Compare June 12, 2025 21:36
@giuliof
Copy link
Contributor

giuliof commented Jun 17, 2025

Is the documentation mandatory to build the emulator?

@giomba
Copy link
Collaborator Author

giomba commented Jun 17, 2025

Is the documentation mandatory to build the emulator?

No, it is useful only for development.

CMakeLists.txt Outdated

include_directories(vendor/Z80/API)
set(Z80_SHARED_LIBS NO CACHE BOOL "")
set(Z80_WITH_HTML_DOCUMENTATION YES)
Copy link
Contributor

Choose a reason for hiding this comment

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

To avoid the documentation to be built by default, I suggest to not set this and to add in the documentation to use -DZ80_WITH_HTML_DOCUMENTATION=True in configuration to optionally build it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I preferred not to have to remember which particular -D flag I had to pass to the build in order to build the documentation, and make it all automagic, but I can still see your point. Ok, I'll just remove it.

@giomba giomba self-assigned this Jun 19, 2025
@giomba giomba force-pushed the giomba/build-libs-docs-in-container branch from c77311f to 83bd443 Compare June 20, 2025 11:19
@giomba giomba requested a review from giuliof June 20, 2025 11:19
@giomba giomba merged commit e917a0b into main Jun 21, 2025
1 check passed
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.

3 participants