Fix line-directive tool to work around Windows command line max limit #6464

hughbe · 2016-12-22T11:07:54Z

Windows has an 8000 character limit for
commands executed from the shell. We exceed this limit when compiling the standard library.

This means that we need to work around this:

Write a list of the files to compile to a file
Read that file in a the new line-directive-file script into a list of files.
Invoke the original line-directive-file via Python.

Extracted from #5904

compnerd

With the @ sigil, it should be possible to merge the tool into the existing line-directive tool, shouldnt it?

compnerd · 2016-12-24T18:47:23Z

cmake/modules/SwiftSource.cmake

+  # Python utility script (line-directive-file), which reads the list of file paths,
+  # passing them to the old Python utility script, line-directive.
+  set(line_directive_tool "${SWIFT_SOURCE_DIR}/utils/line-directive-file")
+  set(line_directive_arguments, "${CMAKE_CURRENT_BINARY_DIR}/line-directive-file.txt")


That , probably is not desired.

I think that it would be nicer to follow the traditional response file naming convention of @name to indicate that the file should be read and expanded.

This was an unused variable (dead code). Removed

compnerd · 2016-12-24T18:48:40Z

cmake/modules/SwiftSource.cmake

+  set(source_files_string "")
+  foreach(file ${source_files})
+    set(source_files_string "${source_files_string} ${file}")
+  endforeach()


I wonder if these 4 lines are used elsewhere in the build system. It really seems ripe for moving into ListUtils.

I've moved it into SwiftList.cmake. I think we can add uses of the new list_join function as we find them

hughbe · 2016-12-26T16:44:03Z

Thanks for the review. For the @name comment, I'm not sure I understand. Could you clarify this? I will update the PR accordingly

slavapestov · 2017-01-11T06:35:45Z

There's a merge conflict here

compnerd

The change that you split out into a wrapper tool can be condensed enough to place into the original tool and simplify things.

compnerd · 2017-01-11T15:41:00Z

cmake/modules/SwiftSource.cmake

+  # Python utility script (line-directive-file), which reads the list of file paths,
+  # passing them to the old Python utility script, line-directive.
+  set(line_directive_tool "${SWIFT_SOURCE_DIR}/utils/line-directive-file")
+  set(file_path "${CMAKE_CURRENT_BINARY_DIR}/line-directive.tmp")


I'm not sure that I like this. Just feels like the could be a collision of there are parallel builds. Can we use a generated name?

compnerd · 2017-01-11T15:43:07Z

cmake/modules/SwiftSource.cmake

          "${swift_compiler_tool}" "-emit-module" "-o" "${module_file}" ${swift_flags}
-          "${source_files}"


I think that I would rather that we keep the old behavior in tact. Can you use "@${file_path}" here?

compnerd · 2017-01-11T15:44:03Z

cmake/modules/SwiftSource.cmake

        "${swift_compiler_tool}" "${main_command}" ${swift_flags}
-        ${output_option} ${embed_bitcode_option} "${source_files}"


I think that I would rather we keep the old behavior. Can you use "@${file_path}" here?

compnerd · 2017-01-11T15:45:13Z

cmake/modules/SwiftSource.cmake

  add_custom_command_target(
      dependency_target
      COMMAND
-        "${line_directive_tool}" "${source_files}" --
+        "${PYTHON_EXECUTABLE}" "${line_directive_tool}" "${file_path}" --


I think that I would rather keep the tool compatible with the previous invocation. Can you use the @ sigil to indicate that the contents of the file should be read as options?

compnerd · 2017-01-11T15:45:31Z

cmake/modules/SwiftSource.cmake

          "${swift_compiler_tool}" "-emit-sib" "-o" "${sib_file}" ${swift_flags} -Onone
-          "${source_files}"


compnerd · 2017-01-11T15:45:37Z

cmake/modules/SwiftSource.cmake

          "${swift_compiler_tool}" "-emit-sibgen" "-o" "${sibgen_file}" ${swift_flags}
-          "${source_files}"


compnerd · 2017-01-11T15:46:49Z

cmake/modules/SwiftSource.cmake

+  # achieved by writing the list of file paths to a file, passing that file to a new
+  # Python utility script (line-directive-file), which reads the list of file paths,
+  # passing them to the old Python utility script, line-directive.
+  set(line_directive_tool "${SWIFT_SOURCE_DIR}/utils/line-directive-file")


I think that the change needed is small enough that we can collapse of into the tool itself.

compnerd · 2017-01-11T15:49:07Z

utils/line-directive-file

+    # that we need to have a way to bypass the command line to pass files to
+    # line-directive. An alternative is reading a file written by CMake and
+    # forwarding the arguments via Python.
+    with open(sys.argv[1], 'r') as files:


Adding a:

if sys.argv[0][0] == '@':

means that we can integrate this into the original tool and makes it behave more like other tools.

compnerd · 2017-01-11T15:50:27Z

utils/line-directive-file

+    if len(sys.argv) > 2:
+        # We need to append the list of files again (for swiftc)
+        args = sys.argv[2:]
+        args.extend(sources)


Doesn't this flip the order? We want the previously read options to precede the subsequent options.

hughbe · 2017-01-15T13:22:08Z

Thanks Saleem for your review. I've updated the PR to your comments.

This PR now depends on #6824 being merged

compnerd · 2017-01-15T19:18:11Z

cmake/modules/SwiftSource.cmake

@@ -518,9 +530,9 @@ function(_compile_swift_files
    add_custom_command_target(
        sibopt_dependency_target
        COMMAND
-          "${PYTHON_EXECUTABLE}" "${line_directive_tool}" "${source_files}" --
+          "${line_directive_tool}" "@${file_path}" --


Don't think that this is right. Why are you dropping the use of ${PYTHON_EXECUTABLE}?

Whoops, mad merge conflict resolution

compnerd · 2017-01-15T19:19:25Z

utils/line-directive

+        command_args[0] = os.path.normpath(command_args[0])
+
+        last_index = len(command_args) - 1
+        if command_args[last_index][0] == '@':


This can be written as if command_args[-1][0] == '@'

compnerd · 2017-01-15T19:23:30Z

utils/line-directive

+        if command_args[last_index][0] == '@':
+            # Read sources from a file.
+            command_args = command_args[0:last_index]
+            command_args.extend(sources)


Is there a reason that we want the response file to be the last entry always? I think that we should expand any and in place.

hughbe · 2017-01-15T20:35:21Z

I've updated the PR to fix the bad merge, and consolidate response file expansion

compnerd · 2017-01-15T22:34:03Z

@swift-ci please test

swift-ci · 2017-01-15T22:34:35Z

Build failed
Jenkins build - Swift Test OS X Platform
Git Commit - f443fff5e8c8b1412c9a0b7e3010e7187168657a
Test requested by - @compnerd

swift-ci · 2017-01-15T22:34:52Z

Build failed
Jenkins build - Swift Test Linux Platform
Git Commit - f443fff5e8c8b1412c9a0b7e3010e7187168657a
Test requested by - @compnerd

hughbe · 2017-01-17T22:11:21Z

@compnerd any more comments or is this good to go?

compnerd · 2017-01-18T02:27:49Z

cmake/modules/SwiftSource.cmake

+  # in the Python script.
+  string(RANDOM file_name)
+  set(file_path "${CMAKE_CURRENT_BINARY_DIR}/${file_name}.txt")
+  list_join("${source_files}" ";" source_files_string)


if you are joining the list with ; (which does make it easier to deal with paths which may have spaces, although it is less like traditional response files), then the list_join is equivalent to "${source_files}" as CMake will implicitly ; the list when converting it to a string.

Yeah, I found that things glitched out and broken in a path with spaces, so replaced with ';'

Actually, I just discovered ";" is a legal character in a Windows file path. I've replaced uses of ';' with '|' which is illegal

What am I misunderstanding? The separator is still ; on the diff.

; is legal on Unix too. I'm pretty sure cmake is hopelessly doomed if any file path actually uses ; so we don't need to work around that threat here.

Derp, I didn't push the commit changing ';' to '|'

Fair enough actually - I'll keep ';' and follow your suggestion

hughbe · 2017-01-18T10:18:10Z

@swift-ci please smoke test

Please re-review

hughbe · 2017-01-25T10:42:17Z

@swift-ci please smoke test

compnerd · 2017-01-26T02:42:55Z

utils/line-directive

@@ -154,6 +154,21 @@ def map_line_from_source_file(source_filename, source_line_num, target_filename)
            return result + 1
    raise RuntimeError("line not found")

+def read_response_file(file_path):
+    with open(file_path, 'r') as files:
+        return filter(None, files.read().split(';'))


hughbe · 2017-01-26T09:35:39Z

@swift-ci please smoke test

compnerd reviewed Dec 24, 2016

View reviewed changes

slavapestov assigned compnerd Jan 9, 2017

compnerd previously requested changes Jan 11, 2017

View reviewed changes

compnerd reviewed Jan 15, 2017

View reviewed changes

compnerd reviewed Jan 18, 2017

View reviewed changes

compnerd reviewed Jan 26, 2017

View reviewed changes

Fix line-directive tool to work around Windows command line max limit

3b4187f

hughbe closed this Jan 26, 2017

hughbe reopened this Jan 26, 2017

compnerd approved these changes Jan 26, 2017

View reviewed changes

compnerd merged commit 466ab59 into swiftlang:master Jan 26, 2017

hughbe deleted the line-directive-windows branch January 26, 2017 15:49

		"${swift_compiler_tool}" "-emit-module" "-o" "${module_file}" ${swift_flags}
		"${source_files}"

		"${swift_compiler_tool}" "${main_command}" ${swift_flags}
		${output_option} ${embed_bitcode_option} "${source_files}"

		"${swift_compiler_tool}" "-emit-sib" "-o" "${sib_file}" ${swift_flags} -Onone
		"${source_files}"

		"${swift_compiler_tool}" "-emit-sibgen" "-o" "${sibgen_file}" ${swift_flags}
		"${source_files}"

Fix line-directive tool to work around Windows command line max limit #6464

Fix line-directive tool to work around Windows command line max limit #6464

Conversation

hughbe commented Dec 22, 2016

compnerd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hughbe commented Dec 26, 2016

slavapestov commented Jan 11, 2017

compnerd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hughbe commented Jan 15, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hughbe commented Jan 15, 2017

compnerd commented Jan 15, 2017

swift-ci commented Jan 15, 2017

swift-ci commented Jan 15, 2017

hughbe commented Jan 17, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hughbe commented Jan 18, 2017

hughbe commented Jan 25, 2017

Choose a reason for hiding this comment

hughbe commented Jan 26, 2017