[driver] Add -working-directory option #14746

benlangmuir · 2018-02-20T19:08:04Z

Adds a -working-directory option which can be used to modify how
relative paths are resolved. It affects all other paths used in driver
options (controlled by a new ArgumentIsPath flag on options) as well as
the contents of output file maps and auxilliary file paths generated
implicitly by the compiler itself.

rdar://37713856

benlangmuir · 2018-02-20T19:08:13Z

@swift-ci please test

benlangmuir · 2018-02-20T19:16:43Z

@jrose-apple I'd appreciate your feedback on this patch. The general approach I took was to make this a driver-only option and eagerly resolve all relative paths in the driver. If you pass -working-directory, the frontend should only see absolute paths.

For now I didn't touch the code in parseArgs that sets the frontend's working directory; we could maybe do that entirely in the driver now by always making all paths absolute before handing them off to the frontend, but I wasn't sure if it was a good idea. It would require a lot of test churn so I didn't go very far down that road.

Another thing: using this option shouldn't affect the incremental build if all your paths were absolute to begin with, but because we add -Xcc -working-directory I'm guessing we have no way to guarantee that, right?

jrose-apple

Seems like it'll work. Only a few comments.

jrose-apple · 2018-02-20T23:49:41Z

lib/Driver/Driver.cpp

+    // although having a different working directory is probably incorrect.
+    auto xcc = Opts->getOption(options::OPT_Xcc);
+    DAL->AddSeparateArg(/*BaseArg=*/nullptr, xcc, "-working-directory");
+    DAL->AddSeparateArg(/*BaseArg=*/nullptr, xcc, workingDirectory);


Please do not do this here. Put it in addCommonFrontendOptions instead.

Is there somewhere I should be stashing the working directory so that it's available directly in that function, or should I get it from the argument list (and call make_absolute) again?

I think doing that is fine.

jrose-apple · 2018-02-20T23:51:20Z

lib/Driver/Driver.cpp

    }
  }
 }

+/// Form a filename based on \p base in \p result, optionally setting its
+/// extension to \p newExt and in \p workingDirectory.
+static void filenameFromBaseAndExt(StringRef base, StringRef newExt,


Nitpick: I think this function name needs a verb if the result isn't in the return value.

jrose-apple · 2018-02-20T23:58:59Z

Another thing: using this option shouldn't affect the incremental build if all your paths were absolute to begin with, but because we add -Xcc -working-directory I'm guessing we have no way to guarantee that, right?

I don't really care about preserving that. I don't see people adding or removing -working-directory from their invocation but using the same build products.

jrose-apple · 2018-02-21T00:20:43Z

Oh, and here are some more options that might refer to paths:

tools_directory
output_file_map_EQ
emit_remap_file_path
emit_migrated_file_path
dump_migration_states_dir
api_diff_data_file
L_EQ

benlangmuir · 2018-02-21T00:57:24Z

I reordered your list:

tools_directory
api_diff_data_file
dump_migration_states_dir

Will do!

L_EQ
output_file_map_EQ

These already work, presumably because they're aliases of options that have the right flags. I will add a test for output-file-map and output-file-map= to verify that. I see in some places we duplicate flags onto Aliases and in others (like here) we don't. I matched the existing pattern when I added ArgumentIsPath.

emit_remap_file_path
emit_migrated_file_path

These are NoDriverOption, so wouldn't be handled by my changes for -working-directory. Do you think I should still add the ArgumentIsPath flag for completeness? I wasn't planning to do this for any of the frontend options.

jrose-apple · 2018-02-21T01:05:19Z

These already work, presumably because they're aliases of options that have the right flags. I will add a test for output-file-map and output-file-map= to verify that. I see in some places we duplicate flags onto Aliases and in others (like here) we don't. I matched the existing pattern when I added ArgumentIsPath.

I don't really care what happens here but I'd like to be consistent for all aliases.

These are NoDriverOption, so wouldn't be handled by my changes for -working-directory. Do you think I should still add the ArgumentIsPath flag for completeness? I wasn't planning to do this for any of the frontend options.

I'm okay with you not doing so. I just wanted to make sure it was an explicit decision.

Adds a -working-directory option which can be used to modify how relative paths are resolved. It affects all other paths used in driver options (controlled by a new ArgumentIsPath flag on options) as well as the contents of output file maps and auxilliary file paths generated implicitly by the compiler itself. rdar://37713856

benlangmuir · 2018-02-21T17:46:09Z

@jrose-apple updated patch per your feedback. For the aliases that I touched, I copied all the flags from the aliased options for consistency.

benlangmuir · 2018-02-21T17:46:25Z

@swift-ci please smoke test

graydon

Double check with @jrose-apple but I think at least the movement to a field in Compilation would be good. Otherwise looks ok to me.

graydon · 2018-02-21T22:08:26Z

include/swift/Driver/Driver.h

-                 const OutputFileMap *OFM, const ToolChain &TC,
-                 Compilation &C) const;
+                 const OutputFileMap *OFM, StringRef workingDirectory,
+                 const ToolChain &TC, Compilation &C) const;


I think I'd prefer to make this a field on the Compilation object. It's of a similar provenance to the other things stored there and it looks like you're having to thread it through lots of calls that already take a Compilation.

Oh fun. I was following the example of OutputFileMap, OutputInfo and ToolChain. Now that I look more carefully at Compilation, I see we have fields for all three of those (and they appear to me to be the same objects), so I don't know why we also thread them through every call too. I'm happy to move working dir into Compilation assuming @jrose-apple agrees.

I don't love it because it seems like something that's only used while building up the Compilation, and not something you need once it's fully-built and ready to, well, compile things. But maybe that distinction's only in my head.

(Originally, none of the three things you mentioned were in Compilation. They've all migrated there because of batch mode, which needs to construct jobs "late".)

The OutputFileMap held in the compilation is a derived one (holding all the aux and intermediates in a compilation), whereas the one being passed around as args here is an input. This is a somewhat recent development and perhaps not ideal, but they're not the same object.

The ToolChain and OutputInfo args are the same as those being threaded around though, it's true; I guess we can just pull them out of Compilation now (they're new arrivals).

Oh, ok. On matters of taste here I'm happy to defer to @jrose-apple, passed-as-an-arg it is!

I'll admit I don't love another argument being passed around either. I'm just not sure Compilation is the place to put it. And Ben doesn't have to be the one to clean this up.

graydon · 2018-02-21T22:15:14Z

lib/Driver/OutputFileMap.cpp

+    PathStorage.insert(PathStorage.begin(), workingDirectory.begin(),
+                       workingDirectory.end());
+    llvm::sys::path::append(PathStorage, PathStrCopy);
+    return StringRef(PathStorage.data(), PathStorage.size());


Unless I'm misunderstanding, I think you're returning a StringRef to a local buffer that goes out of scope here (i.e. invalid memory).

Whoops, yes, I misread.

benlangmuir requested a review from jrose-apple February 20, 2018 19:16

jrose-apple reviewed Feb 20, 2018

View reviewed changes

jrose-apple requested a review from graydon February 20, 2018 23:58

benlangmuir force-pushed the working-directory branch from a7be8af to 20cb3e3 Compare February 21, 2018 17:44

graydon suggested changes Feb 21, 2018

View reviewed changes

graydon approved these changes Feb 21, 2018

View reviewed changes

benlangmuir merged commit d991452 into swiftlang:master Feb 21, 2018

benlangmuir deleted the working-directory branch February 21, 2018 22:57

[driver] Add -working-directory option #14746

[driver] Add -working-directory option #14746

Uh oh!

Conversation

benlangmuir commented Feb 20, 2018

Uh oh!

benlangmuir commented Feb 20, 2018

Uh oh!

benlangmuir commented Feb 20, 2018

Uh oh!

jrose-apple left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jrose-apple commented Feb 20, 2018

Uh oh!

jrose-apple commented Feb 21, 2018

Uh oh!

benlangmuir commented Feb 21, 2018

Uh oh!

jrose-apple commented Feb 21, 2018

Uh oh!

benlangmuir commented Feb 21, 2018

Uh oh!

benlangmuir commented Feb 21, 2018

Uh oh!

graydon left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!