Move version detection and compiler flags into utilities.cmake

[Neumann-A]

No description provided.

[BillyONeal]

If anything I don't like that utilities.cmake exists; I think separating this stuff into a separate file just makes it more confusing to follow how our build works given that we never expect those bits to be reused elsewhere. But I was not the 'architect' of the current situation (I believe that was @strega-nil ) so I'm not sure if this change is in keeping with the intended status quo or opposed to it.

Can you explain what the intended benefit of moving this content to utilities.cmake is?

Thanks for your contribution!

[Neumann-A]

Can you explain what the intended benefit of moving this content to utilities.cmake is?

Yes see below:

I think separating this stuff into a separate file just makes it more confusing to follow how our build works

I my opinion its exactly the other way around. The CMakeLists.txt is overloaded with irrelevant stuff and misses a proper structure for the relevant definition of the build targets. Furthermore, add_subdirectory is avoided which makes it even more difficult to follow and find the relevant parts. strega-nil came up with the design before a number of other targets were added to the CMakeLists.txt. Why is the artifact stuff in the same CMakeLists.txt as the rest? Why are the tests not properly separated?

If I express the current state of the CMakeLists.txt in c++ it would be int main() { <all the raw code> } without defining any functions or abstractions which makes it human readable.

[BillyONeal]

Furthermore, add_subdirectory is avoided which makes it even more difficult to follow and find the relevant parts

There doesn't appear to be any subdirectory below the root to which anything could be restricted. CMake loses its mind whenever a target mentions something 'above' the current CMakeLists.txt, and we put 'src' and 'include' in separate top-level directories. (I don't like that either, but just hasn't been a can of worms I've been willing to spend energy opening)

Moreover, the outputs all need to end up built into the same directory, I believe CMake's default behavior with add_subdirectory will be to replicate that directory structure into the output, which would be wrong.

Why is the artifact stuff in the same CMakeLists.txt as the rest?

It must be built to the same place or the result will be broken because the exe finds where the artifacts live by looking relative to where the exe is.

Why are the tests not properly separated?

Once upon a time vcpkg-test also contained e2e stuff and cared about the directory to which it was built, but now I believe it could be built elsewhere. Ditto the other mini test exes, though of course corresponding test changes would need to be made to respond to being built into different locations.

If I express the current state of the CMakeLists.txt in c++ it would be int main() { } without defining any functions or abstractions which makes it human readable.

I agree that functions could reduce a lot of the copy pasta. I don't agree that putting those functions into a separate file buys anything unless the files are understandable on their own, and I don't believe utilities.cmake achieves that. For utilities.cmake to make sense you have to read it in the context of the root CMakeLists.txt, and vice versa, and being split into separate files just makes finding where a given change has to be made more difficult.

But again, I'm speaking as me personally, not as 'the will of the vcpkg maintainers' here.

[Neumann-A]

CMake loses its mind whenever a target mentions something 'above' the current CMakeLists.txt

Since when? Never observed any issues with that.

Moreover, the outputs all need to end up built into the same directory, I believe CMake's default behavior with add_subdirectory will be to replicate that directory structure into the output, which would be wrong.

There are ways to control all build output folders (basically all x_OUTPUT_DIRECTORY variables / target properties)

anything unless the files are understandable on their own / to make sense you have to read it in the context of the root

The contents of vcpkg_setup_compiler_flags are not needed for understanding the CMakeLists.txt; strictly these are toolchain changes which don't even belong here.

The contents of vcpkg_define_version are not needed for understanding the build of the CMakeLists.txt. You could argue it should have a proper function signature so that the output variables are visible to the caller. The variables are just placeholders for arbitrary numbers which could even be passed via the cmake cmd line. The only reason it exists in the cmake files is for convenience.

In the end these changes here are for humans. Shorter files mean faster skimming through files. Physical separation of code into different files in general means that code is in some way logical separated/grouped. This in return reduces the mental load on the reader.

[ras0219-msft]

tldr; I agree that at some level of buildsystem complexity it becomes important to separate and organize, but I don't think the vcpkg-tool build has reached the tradeoff point yet (currently at <700 lines).

I think that the nature of the CMake language (lots of imperative, global state, without proper function returns) makes it nearly impossible to effectively encapsulate concerns. When debugging or making improvements, you cannot ignore the function call vcpkg_detect_compiler or vcpkg_setup_compiler_flags because these can change your entire local context. In my opinion, this heavily shifts the balance towards the "Make it as easy as possible to read everything in straight-line execution order with as few GOTOs as possible" and away from "Try to introduce firewalls between components with clear separation of concerns."

Given the above, I'd agree with the following changes:

• Adding a Table of Contents comment at the top with easily searchable terms to make it easier to ctrl-F to the section you are interested in, completely avoiding an O(N) eyeball scan. This could list section names such as "Reused Functions", "System Detection", "Compiler Flags", "Binary Targets", "Logical Targets", etc.
• Wrapping chunks of logic in functions (not macros) which are immediately called. The file should still be readable in execution order from top to bottom, but since functions introduce variable scope we get some encapsulation and clarity of outputs.
• utilities.cmake should be removed and inlined into the main CMakeLists.txt
• Moving the CPP_ATOMIC_LIBRARY detection up to the "System Detection" section and out of the middle of the targets

I'd be interested in reviewing a change that rearranges parts of the file in a way that has clearly articulated principles -- perhaps separating binary targets (like vcpkglib) from "maintenance" targets (like verify-messages or format) from logical targets (vcpkg-artifacts-target and npm-restore). But it is essential that any such reorganization has clear rules about what goes where and why (for example, putting all the target_link_libraries calls at the end of the file is a clear rule but I don't see any benefit).