[NFC] Eliminate duplicated create_dirs

[stevapple]

This will slightly improve build performance. Most importantly, it unblocks the limit of having the same output directory in parallel.

Resolves SR-NNNN.

[stevapple]

Anyone to review & test?

[stevapple]

Ping again

[stevapple]

Ping

[stevapple]

Since I don’t know who’s exactly in charge of CMake configurations, @airspeedswift @jckarter can you review this?

[edymtt]

I should be able to help you out with this.

Could you expand on the rationale for this change? From a quick skim, this seems to be breaking up the creation of the directories in multiple targets, but I cannot see what problem this is trying to solve (even after looking at #41785).

In the meantime I will try to have a look at differences in the generated ninja files.

(Please note that I have other projects to attend to, so I may not follow up immediately with this)

[stevapple]

@edymtt CMake will fail to resolve when we add_swift_target_library multiple times within the same component in a file.

After looking into it a bit, I found that CMake regards duplicated create_dirs job as “the same operation that needs to be executed multiple times”. Since CMake also assumes that jobs are effective, it cannot decide when to execute its dependents. eg.,

job1 (2x) -> job2 // where to put job2: after the first execution of job1 or the second?

[etcwilde]

What is the error message you're seeing and how do you reproduce it? As far as I can tell, this patch is gating the creation of a target on a cache variable. You're only allowed to create a target once anyway and duplicates result in an error. I don't see the error if I only pull in commit d544bbb from #41785 and configure.
If the target is depended on by multiple dependers, it will get run once before any of them.

[stevapple]

Did you test it on Windows? Which CMake version are you using?

> cmake --version
cmake version 3.23.0-rc1

CMake suite maintained and supported by Kitware (kitware.com/cmake).

> cmake -B S:\b\1 ^
  -C S:\swift\cmake\caches\Windows-x86_64.cmake ^
  -D CMAKE_BUILD_TYPE=Release ^
  -D CMAKE_INSTALL_PREFIX=S:\b\toolchain\usr ^

  -D CMAKE_C_COMPILER=cl.exe ^
  -D CMAKE_C_FLAGS="/GS- /Oy /Gw /Gy /source-charset:utf-8 /execution-charset:utf-8" ^
  -D CMAKE_CXX_COMPILER=cl.exe ^
  -D CMAKE_CXX_FLAGS="/GS- /Oy /Gw /Gy /utf-8" ^
  -D CMAKE_MT=mt ^
  -D CMAKE_EXE_LINKER_FLAGS="/INCREMENTAL:NO" ^
  -D CMAKE_SHARED_LINKER_FLAGS="/INCREMENTAL:NO" ^

  -D LLVM_DEFAULT_TARGET_TRIPLE=x86_64-unknown-windows-msvc ^
  -D LLVM_ENABLE_PDB=YES ^

  -D LLVM_EXTERNAL_CMARK_SOURCE_DIR=S:\cmark ^
  -D LLVM_EXTERNAL_SWIFT_SOURCE_DIR=S:\swift ^
  -D SWIFT_PATH_TO_LIBDISPATCH_SOURCE=S:\swift-corelibs-libdispatch ^

  -D SWIFT_ENABLE_EXPERIMENTAL_CONCURRENCY=YES ^
  -D SWIFT_ENABLE_EXPERIMENTAL_DISTRIBUTED=YES ^
  -D SWIFT_ENABLE_EXPERIMENTAL_DIFFERENTIABLE_PROGRAMMING=YES ^

  -D SWIFT_ENABLE_EXPERIMENTAL_STRING_PROCESSING=YES ^
  -D EXPERIMENTAL_STRING_PROCESSING_SOURCE_DIR=S:\swift-experimental-string-processing ^

  -G Ninja ^
  -S S:\llvm-project\llvm

……

CMake Error at S:/swift/cmake/modules/SwiftAddCustomCommandTarget.cmake:147 (add_custom_target):
  add_custom_target cannot create target
  "tools-swift-stdlib-public-Platform-WINDOWS-x86_64" because another target
  with the same name already exists.  The existing target is a custom target
  created in source directory
  "S:/swift/stdlib/public/Platform".  See
  documentation for policy CMP0002 for more details.
Call Stack (most recent call first):
  S:/swift/stdlib/cmake/modules/SwiftSource.cmake:852 (add_custom_command_target)
  S:/swift/stdlib/cmake/modules/SwiftSource.cmake:133 (_compile_swift_files)
  S:/swift/stdlib/cmake/modules/AddSwiftStdlib.cmake:870 (handle_swift_sources)
  S:/swift/stdlib/cmake/modules/AddSwiftStdlib.cmake:2076 (add_swift_target_library_single)
  S:/swift/stdlib/public/Platform/CMakeLists.txt:117 (add_swift_target_library)

[etcwilde]

So the cache variable is definitely not what you want. If you re-run, I suspect you'll see a bunch of errors around missing targets since once you set it, you'll have to go into your cmake cache and remove the variable if you ever want to see it again.

Now, in understanding the whole change that you've put up here, why does moving a file cause everything to fail? It's the same call being made, whether it's from stdlib/public/Windows/CMakeLists.txt or from stdlib/public/Platform/CMakeLists.txt as the change proposes?

[stevapple]

I suspect that’s caused by being in the same file (and the same block) with swiftCRT, another module yet in the same component.

Also, I didn’t notice anything wrong with re-runs. I’m using this patch locally and have reconfigured and rebuilt Swift for multiple times across the latest months. Everything looks okay.

[stevapple]

If you re-run, I suspect you'll see a bunch of errors around missing targets since once you set it, you'll have to go into your cmake cache and remove the variable if you ever want to see it again.

However, it’s true that there’re potential risks when a create_dirs target is not executed but being removed through re-configuration. It’s rather easy to fix: set the cache to blank list in CMakeList.txt, which is the main entry and everything on top will be executed exactly once.

[edymtt]

This seems a legit bug where _compile_swift_file adds a target to create a build support folder without checking if such target already exist -- probably that was never hit before since, as @stevapple noted, so far we had exactly one add_swift_target_library executed per platform/folder, and as such the computed support folder path only account for those two variables and "ignores" the modules name.

I would advise against using a cache variable in this scenario -- this can be solved by modelling this directly in CMake, by either

1. recognizing a proper target has been created and using that when creating the subsequent target
2. or changing the support folder names to include the module name

(that's assuming that #41785 is not negotiable -- if the WinSDK.swift overlay remains in a separate folder, we don't hit this issue)

The former option seems easier to implement and test with the facilities we have -- I proposed some untested code that should achieve that.

[edymtt]

This would be my untested proposal that reuses previously already created targets, instead of relying on a cache variable (that would make us lose the dependency for the subsequent targets)

Suggested change

	list(REMOVE_ITEM dirs_to_create ${CREATED_OBJ_DIRS})
	if(NOT dirs_to_create STREQUAL "")
	add_custom_command_target(
	create_dirs_dependency_target
	COMMAND "${CMAKE_COMMAND}" -E make_directory ${dirs_to_create}
	OUTPUT ${dirs_to_create}
	COMMENT "Generating dirs for ${first_output}")
	set(CREATED_OBJ_DIRS ${CREATED_OBJ_DIRS} ${dirs_to_create} CACHE INTERNAL "Already created obj directories")
	endif()
	list(APPEND create_dirs_dependency_targets)
	foreach(current_dir_to_create IN LISTS dirs_to_create)
	add_custom_command_target(
	create_dirs_dependency_target
	IDEMPOTENT
	CUSTOM_TARGET_NAME ${current_dir_to_create}
	COMMAND "${CMAKE_COMMAND}" -E make_directory ${current_dir_to_create}
	OUTPUT ${current_dir_to_create}
	COMMENT "Generating dirs for ${first_output}")
	list(APPEND create_dirs_dependency_targets "$create_dirs_dependency_target")
	endforeach()
	# use create_dirs_dependency_targets below instead of create_dirs_dependency_target

This likely could be simplified more (e.g. from the build ninja it seems only one directory is created), erring on the side of caution without doing builds/PR testing.

Another solution would be to alter the temporary path name to account for the module name, but that seems a slightly more invasive solution.

[stevapple]

(that's assuming that #41785 is not negotiable -- if the WinSDK.swift overlay remains in a separate folder, we don't hit this issue)

For a clearer source layout, it would be better for us to put overlays for the same platform together. Of course that's not a key change, but I think the current CMake script does have bugs that we should try to resolve.

[stevapple]

Closed along with #41785