Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement @argsfile to read arguments from command line #63576

Closed
jsgf opened this issue Aug 14, 2019 · 15 comments

Comments

@jsgf
Copy link
Contributor

@jsgf jsgf commented Aug 14, 2019

Many tools, such as gcc and gnu-ld, support "args files" - that is, being able to specify @file on the command line.

Implemented in #63175

This implements a straightforward form where each argument is on a separate line in a text file. This allows for everything except for arguments with internal line breaks.

Description (also in docs)

If you specify @path on the command-line, then it will open path and read
command line options from it. These options are one per line; a blank line indicates
an empty option. The file can use Unix or Windows style line endings, and must be
encoded as UTF-8.

@hellow554

This comment has been minimized.

Copy link
Contributor

@hellow554 hellow554 commented Aug 15, 2019

You probably mean #63175 because your link PR is just the rollup.
Can you please clarify that this is a tracking issue? You can take #63569 as an example (or anything else with the C-tracking-issue label)

@jsgf

This comment has been minimized.

Copy link
Contributor Author

@jsgf jsgf commented Aug 28, 2019

Does this need anything else to be a complete tracking issue? This is a relatively minor feature, so there's no RFC or anything to reference.

@Mark-Simulacrum

This comment has been minimized.

Copy link
Member

@Mark-Simulacrum Mark-Simulacrum commented Oct 18, 2019

@jsgf I seem to recall you wanting to use this feature within FB or so, have you had a chance to do so? Do we have any usage reports; not technically necessary but good for stabilization, I think.

@jsgf

This comment has been minimized.

Copy link
Contributor Author

@jsgf jsgf commented Oct 18, 2019

@Mark-Simulacrum Yes, I'm using this regularly and haven't had any problems with it. I'd like to stabilize it.

@Mark-Simulacrum

This comment has been minimized.

Copy link
Member

@Mark-Simulacrum Mark-Simulacrum commented Oct 18, 2019

@rfcbot fcp merge

I propose that we stabilize this feature. It adds support for arg files via @file on the command line, similar to other tools. We have seen both @cramertj and @jsgf state that they want this, and @jsgf confirms that this has worked well in their use.

We differ from ldd and other tools in that:

  • We do not support recursive use of @file (i.e., we expand "once")
  • We separate by newline, rather than by whitespace. This is easier to parse and avoids quoting (we do not support escaping newlines, but it is super rare to want a newline anyway).

These changes are due to wanting to simplify the option and hopefully modernize it some -- we felt that including support would complicate the code and be unnecessary. You can find some ~extensive discussion on the implementation PR (see #63175).

(I am uncertain whether this merge command will work, due to possibly lacking permissions, but if so I'll bug someone to rerun it).

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

@nikomatsakis nikomatsakis commented Oct 21, 2019

@rfcbot fcp merge

Quoting @Mark-Simulacrum:

I propose that we stabilize this feature. It adds support for arg files via @file on the command line, similar to other tools. We have seen both @cramertj and @jsgf state that they want this, and @jsgf confirms that this has worked well in their use.

We differ from ldd and other tools in that:

  • We do not support recursive use of @file (i.e., we expand "once")
  • We separate by newline, rather than by whitespace. This is easier to parse and avoids quoting (we do not support escaping newlines, but it is super rare to want a newline anyway).

These changes are due to wanting to simplify the option and hopefully modernize it some -- we felt that including support would complicate the code and be unnecessary. You can find some ~extensive discussion on the implementation PR (see #63175).

(I am uncertain whether this merge command will work, due to possibly lacking permissions, but if so I'll bug someone to rerun it).

@rfcbot

This comment has been minimized.

Copy link

@rfcbot rfcbot commented Oct 21, 2019

Team member @nikomatsakis has proposed to merge this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@oli-obk

This comment has been minimized.

Copy link
Contributor

@oli-obk oli-obk commented Oct 21, 2019

We do not support recursive use of @file (i.e., we expand "once")

Are we forward compatible with adding recursive use? It seems to me like adding that later would be a breaking change. Could we error right now on recursive uses?

@Mark-Simulacrum

This comment has been minimized.

Copy link
Member

@Mark-Simulacrum Mark-Simulacrum commented Oct 21, 2019

We do not support recursive use of @file (i.e., we expand "once")

Are we forward compatible with adding recursive use? It seems to me like adding that later would be a breaking change. Could we error right now on recursive uses?

It would be no more a breaking change than this -- i.e., we would mostly error today on such a case since @file likely doesn't exist. This patch already "breaks" code that uses @file, but the fix is trivial, use ./@file, so this is deemed acceptable (starting filenames with @ is expected to be super rare).

However, we can likely add a check fairly easily that the recursive usage would definitely not be a breaking change if you think that's warranted.

@oli-obk

This comment has been minimized.

Copy link
Contributor

@oli-obk oli-obk commented Oct 21, 2019

However, we can likely add a check fairly easily that the recursive usage would definitely not be a breaking change if you think that's warranted.

I think it would be good to be conservative if the check is simple to create

@jsgf

This comment has been minimized.

Copy link
Contributor Author

@jsgf jsgf commented Oct 21, 2019

I originally handled recursion and it was rejected as being two complicated. The questions it raises are:

  1. How do you quote a filename starting with @ if you don't want to include it as args? Answer: use ./@foo, like on the real command line
  2. What's a reasonable upper recursion bound? I originally chose 8.
  3. How does pathname resolution work? Is it relative to the cwd or to the file in which the @ directive was found?

The third point is the most complex. ld, etc, always resolve them relative to the current working directory, however most other recursive include-like mechanisms resolve relative to the including file. I can think of circumstances in which either is going to be the preferred behaviour, but I definitely wouldn't want to have any syntax or other mechanism to switch it.

I think the conclusion is that recursive use isn't important in practice, and even though questions 1 & 2 have reasonable answers, 3 is complicated enough to cast doubt on the whole thing. Besides, these files are typically machine generated, so when generating one you can flatten them - if you want recursion, read the file and expand it in-place.

Edit: I remember there was an irritating 4: What is the file encoding? It turns out its incredibly awkward to read an arbitrary text file and use text from it as a pathname, esp on Windows. It basically requires insisting that the file is UTF-8, but pathnames are not necessarily UTF-8 or even encodable with it.

@oli-obk

This comment has been minimized.

Copy link
Contributor

@oli-obk oli-obk commented Oct 21, 2019

Ok, seems reasonable to me to never include recursion, thanks for the detailed explanation

@rfcbot

This comment has been minimized.

Copy link

@rfcbot rfcbot commented Oct 26, 2019

🔔 This is now entering its final comment period, as per the review above. 🔔

@rfcbot

This comment has been minimized.

Copy link

@rfcbot rfcbot commented Nov 5, 2019

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

The RFC will be merged soon.

jsgf added a commit to jsgf/rust that referenced this issue Nov 7, 2019
Centril added a commit to Centril/rust that referenced this issue Nov 8, 2019
Stabilize @file command line arguments

Issue rust-lang#63576
Centril added a commit to Centril/rust that referenced this issue Nov 9, 2019
Stabilize @file command line arguments

Issue rust-lang#63576
Centril added a commit to Centril/rust that referenced this issue Nov 9, 2019
Stabilize @file command line arguments

Issue rust-lang#63576
mark-i-m added a commit to mark-i-m/rust that referenced this issue Nov 12, 2019
@jsgf

This comment has been minimized.

Copy link
Contributor Author

@jsgf jsgf commented Nov 19, 2019

Now stable.

@jsgf jsgf closed this Nov 19, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.