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

Use "" (empty argument) to indicate no file after -run #7927

Closed
wants to merge 1 commit into from

Conversation

marler8997
Copy link
Contributor

@marler8997 marler8997 commented Feb 20, 2018

Fix issue 18477

Currently, -run must appear at the end of the command line and be followed by a single source file and then the arguments to pass to the compiled program, i.e.

dmd <files/options>... -run <file> <arg>...
# EXAMPLE
dmd foo.d bar.d -run baz.d <arg>...

This interface makes it difficult for tools to take a list of source files and pass them to dmd with the -run option, i.e.

FILES=foo.d bar.d baz.d
dmd ???<insert-bash-vodo>???

For myself and others, this interface is confusing. Having to put exactly one source file after -run followed by the compiled program arguments is unintuitive. It requires that one of the files be "split up" from the rest and moved behind the -run argument. I'm not sure why it was designed this way, my best guess is that it is supposed to indicate which source file has the main function, but DMD already knows this which violates the DRY principle, and furthermore, DMD doesn't enforce this requirement.

It's too late to change this interface and break tools that use it this way, however, I propose we support -- as a special argument after -run to indicate "no file".

dmd foo.d bar.d baz.d -run -- <arg1> <arg2> ...

This allows all source files to be given before -run while maintaining compatibility with the original interface. It's also unambiguous. Note that supporting a special argument to mean "no file" would be required even if we wanted to make the file argument completely optional (i.e. dmd foo.d -run arg1 arg2...). For example, if we just made the file optional, then we would need to define a mechanism to determine when the first argument after -run is a file meant for the compiler. Assume that "mechanism" is when the argument has a .d extension. But then when we want to pass an argument with a .d extension as the first argument to the compiled program, we will need to come up with a mechanism to negate the original "mechanism", which is why we would still need to define a special argument to indicate "no file". So in any case, if we want to support this and be backwards compatible, we must support a special argument that means "no file".

So just note that this change improves some use cases for -run without superseding future improvements, such as making the file after -run optional or creating a new command-line option that does not take a file but only arguments to the command-line program. In other words, it is the minimum change set needed to make -run more usable without breaking compatibility.

@dlang-bot
Copy link
Contributor

Thanks for your pull request and interest in making D better, @marler8997! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please verify that your PR follows this checklist:

  • My PR is fully covered with tests (you can see the annotated coverage diff directly on GitHub with CodeCov's browser extension
  • My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
  • I have provided a detailed rationale explaining my changes
  • New or modified functions have Ddoc comments (with Params: and Returns:)

Please see CONTRIBUTING.md for more information.


If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment.

Bugzilla references

Auto-close Bugzilla Severity Description
18477 normal -run isn't DRY and leads to unexpected errors

@marler8997 marler8997 force-pushed the dashRunMoreDry branch 3 times, most recently from 2acbd92 to 13026a7 Compare February 20, 2018 20:26
function echorun() {
echo $@ > ${output_file}
$@
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the motivation for this echorun?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It runs the command and logs the command to the output_file

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Logging the output is simple with tee:

| tee ${output_file} | ...

If you want to log all arguments, -x is your friend.
You can even dump the entire debug stream to the output_file:

exec 42> ${output_file}
export BASH_XTRACEFD=42

(42 is arbitrary)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it logs the command, not the output of the command...how would you use tee for that?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NOTE: I removed this in favor of #7928 and possible future improvements to support "logging by default" to BASH tests.

$@
}

out=$(echorun ${DMD} -conf= -m${MODEL} compilable/extra-files/echoargs.d -run a b c)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you use -conf= , Phobos shouldn't be available...

}

out=$(echorun ${DMD} -conf= -m${MODEL} compilable/extra-files/echoargs.d -run a b c)
[ "$out" == "a b c" ] || exit 1
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

... | grep -q "a b c" would work too.

Copy link
Contributor Author

@marler8997 marler8997 Feb 21, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks like grep works better. Windows keeps the trailing \r which makes == fail.

@marler8997 marler8997 changed the title Allow -run as last argument Use -- to indicate no file after -run Feb 20, 2018
@marler8997 marler8997 force-pushed the dashRunMoreDry branch 6 times, most recently from 423e49f to 3116b26 Compare February 21, 2018 02:19
@JinShil
Copy link
Contributor

JinShil commented Feb 22, 2018

I don't object to this feature, and I may support it, but only after I get clarification on the following,

I'm seeing a lack of principled design in our tools and CLI. There appears to be feature overlap between what -run does and what rdmd does. Instead of further building on the -run feature, perhaps it should eventually be deprecated, delegating that functionality to a tool like rdmd or a 3rd-party build tool.

My understanding is @WalterBright is the keeper of the command line arguments, and I'd like for him to weigh in on what his design principles and intent are. Is -run a historical mistake that we should discourage, or is it an essential feature to make DMD a convenient tool for programmers? Marking this as "Need Approval" until he or @andralex weighs in.

@timotheecour
Copy link
Contributor

@JinShil dmd -run and rdmd both have its use cases and we shouldn't impede on improving the former. eg, dmd -run doesn't do any caching like rdmd does (meant as side effect free on file system); there are other differences. I use both, depending on task

@marler8997
Copy link
Contributor Author

marler8997 commented Feb 22, 2018

Was looking through the commits/PRs to see when -run was added. It looks like it was added way back in 2009! It's in commit 1e8aa7a and there's no PR for it so must have been before github. It looks like it originally only took args as well (no source file). It wasn't until commit dad5a36 that the srcfile arg was added as a requirement after -run. Unfortunately all the commits have no messages and no PRs to reference so I'm not sure why it was added.

@marler8997 marler8997 force-pushed the dashRunMoreDry branch 2 times, most recently from 5b70ea2 to 813e322 Compare March 3, 2018 04:59
@wilzbach
Copy link
Member

My understanding is @WalterBright is the keeper of the command line arguments, and I'd like for him to weigh in on what his design principles and intent are.

AFAICT is @CyberShadow the expert on command line questions.

Two small points

  • dub uses -- too.
  • doesn't -- imply -run?

@CyberShadow
Copy link
Member

CyberShadow commented Mar 11, 2018

Thanks for the ping.

This looks like a misuse of the general convention of what -- means. Usually it means, "options after this are all positional arguments and are not switches, even if they begin with -".

Consider, for example, that you want to compile some files that are in a directory that starts with -. Using dmd -dir/foo.d won't work, but, should dmd implement the conventional use of --, dmd -- -dir/foo.d should. For this reason, I think that using -- here is not the best choice.

- usually means "read from stdin", so not a good option either.

How about an empty string? It's somewhat esoteric, but it should be no problem to use in shell scripts etc.

@marler8997
Copy link
Contributor Author

This looks like a misuse of the general convention of what -- means. Usually it means, "options after this are all positional arguments and are not switches, even if they begin with -".

Ah ok. I see that if we used -- in this case then we would be circumventing the ability to be able to use it in the "conventional way". An "empty argument" sounds like a reasonable idea. I'll go ahead and change it to use that instead.

@marler8997 marler8997 changed the title Use -- to indicate no file after -run Use "" (empty argument) to indicate no file after -run Mar 12, 2018
@timotheecour
Copy link
Contributor

empty arg is not standard (does any other tool uses that?)
another option is using another flag altogether instead of -run eg: -args
In any case, anything would be better than current situation

@wilzbach
Copy link
Member

This looks like a misuse of the general convention of what -- means. Usually it means, "options after this are all positional arguments and are not switches, even if they begin with -".

As mentioned we already have one precedent in our core tools:

dub run mypackage -- <program arguments here>

It's too late to change this interface and break tools that use it this way, however, I propose we support -- as a special argument after -run to indicate "no file".

Hmm I was just thinking about this and if we go with optionally supporting --
We could do the following when -run is seen: search for exact -- in the remaining arguments, if found split the and toggle a flag to mark run mode and save the runtime arguments, otherwise use the existing behavior.

BTW there is yet another thread about people getting confused that -run is position-dependent: https://forum.dlang.org/thread/ddyuldfgstkifmyltbda@forum.dlang.org

@marler8997
Copy link
Contributor Author

@CyberShadow what do you think of @wilzbach's idea? Here's the 2 options we have right now:

Option 1 (Current PR)

use "" to indicate "no file". -run must appear after all other compiler args

dmd <compiler-args>... -run "" <program-args>

Option 2 (@wilzbach idea)

use -- to separate compiler args and program args, -run can appear anywhere in compiler args

dmd <compiler-args>... -run <compiler-args>... -- <program-args>

@CyberShadow
Copy link
Member

Well, I don't have a strong opinion on this, but I generally wouldn't call Dub as a pinnacle of good design. Unless there exists wide-spread precedent that I'm not aware of, for all we know, its use of -- might stem from a misinterpretation of its canonical usage, so reusing it here would just be repeating Dub's mistake.

@marler8997
Copy link
Contributor Author

@timotheecour messaged me on slack to point out that @wilzbach's idea breaks the use case where a program would have used -- as a program argument, i.e.

dmd -run main.d foo -- bar

This would have 2 different meanings before and after implementing -- as a way to separate compiler and program arguments.

This incompatibility, along with the fact that this PR can be "orthogonal" to @wilzbach's idea makes me think this PR is justifiable on it's own.

@marler8997
Copy link
Contributor Author

marler8997 commented Mar 14, 2018

@timotheecour suggested another variation would be to use an option like -arg to "skip" the extra source file after -run instead of an empty argument, i.e.

dmd <compiler-args> -run -args <program-args>

# instead of

dmd <compiler-args> -run "" <program-args>

I'm fine with this.

@timotheecour
Copy link
Contributor

timotheecour commented Mar 14, 2018

advantage of -run -args (or even a single argument-runargs (my own personal favorite option) over -run "" is that empty arg "" will potentially cause issues in more complex scenarios where the command line is built on the shell (eg printing out command line without proper quoting will erase the empty argument); another argument is that -args is more easily searchable , whereas empty argument is not.
to summarize: I'd be ok with:

dmd dmd_args... -runargs prog_args... (or another name eg -args) # my favorite option

or:

dmd dmd_args... -run -args prog_args...  # acceptable option

@CyberShadow
Copy link
Member

dmd dmd_args... -run -args prog_args...

Note that this is currently valid syntax:

$ echo 'module args; void main() {}' > -args.d 
$ touch mod.d
$ dmd mod -run -args foo # OK

@timotheecour
Copy link
Contributor

interesting point, i thought dmd would reject file names starting with -
IMO that -args.d is such a corner case that it'd be ok to break as noone will likely depend on that, but either way that's an argument in favor of my single argument option:
dmd dmd_args... -runargs prog_args... #or any other name, eg -args
which has no ambiguity

@wilzbach
Copy link
Member

Hmm, after looking at -run's behavior in detail, I think it's super easy to get wrong by the user:

Custom flags after -run are silently ignored

because they are program arguments now:

cat > foo.d << EOF
void main(){
    import std.stdio;
    version(Foo)
    {
        "foo".writeln;
    }
    else
    {
        "bar".writeln;
    }
}
EOF

So depending on the position of -run, the user gets three different behaviors - ain't that fun?

> dmd -run foo.d -version=Foo
bar
> dmd -version=Foo -run foo.d
foo
> dmd -run -version=Foo foo.d
Error: module `-version=Foo` is in file '-version=Foo.d' which cannot be read
import path[0] = /usr/include/dlang/dmd

Multiple files

cat > foo.d << EOF
void main(){
    import bar;
    hello();
}
EOF
cat > bar.d << EOF
void hello()
{
    import std.stdio;
    "hello".writeln;
}
EOF
> dmd foo.d bar.d -run
Error: argument expected for switch '-run'
       run 'dmd -man' to open browser on manual
> dmd foo.d -run bar.d
hello
> dmd -run foo.d bar.d
 dmd -run foo.d bar.d
foo.o:foo.d:function _Dmain: error: undefined reference to '_D3bar5helloFZv'
collect2: error: ld returned 1 exit status
Error: linker exited with status 1
> dmd -run bar.d foo.d
/usr/lib/gcc/x86_64-pc-linux-gnu/7.3.1/../../../../lib/Scrt1.o:init.c:function _start: error: undefined reference to 'main'
collect2: error: ld returned 1 exit status
Error: linker exited with status 1
> dmd bar.d -run foo.d
hello
> dmd bar.d foo.d -run
Error: argument expected for switch '-run'
       run 'dmd -man' to open browser on manual

So ideally when -run is added anywhere it should turn DMD in a rdmd-esque behavior and not throw silly errors. However, it looks like we can't do this easily so maybe the following:

  • allow -run at the end (then obviously no args are passed to the program)
  • -run should accept flags after it e.g. (-run -m32 foo.d should work - rdmd does a similar detection)

Now regarding the division of runtime / compiler arguments, I still think that -- is best because it's clear that the arguments after that are meant for the program.
As an advantage of using --, the position of -run can be arbitrary in the compiler arguments (it shouldn't be too hard to detect which of the passed module has the main function)

@CyberShadow
Copy link
Member

Now regarding the division of runtime / compiler arguments, I still think that -- is best because it's clear that the arguments after that are meant for the program.

This is not a good idea because at some point we may want to implement -- with the meaning used in POSIX tools, and we won't be able to because it already means something else.

@marler8997
Copy link
Contributor Author

@wilzbach

I agree with your points but the change you propose breaks compatibility. If we make any changes to -run they should be backwards compatible (unless Andrei or Walter think it's ok to break it for an improved interface like what you propose), however, I don't think they will approve.

For your proposed interface, we would probably need a new option, maybe something like -r? And to separate compile time/runtime arguments, we could use -- or another option like -args as @CyberShadow points out that using -- would prevent us from using its conventional meaning in the future.

If we decide to keep -run compatible, then this change is only an improvement to that interface that does not break compatibility.

@ibuclaw
Copy link
Member

ibuclaw commented Mar 29, 2018

The use of -- appears in many applications that delegate to a subprocess to mean: all arguments that follow will be passed to the subprocess verbatim. Using -args may require a terminator such as \; if the intent is to have it in any position on the command line.

@wilzbach
Copy link
Member

So as rdmd doesn't look like it's getting support for the new -ianytime soon and I use rdmd heavily for quickly running tests and this isn't moving anywhere, I thought I share my current workaround.
Maybe interesting to someone else too who wants to profit from not interpreting the file twice, but still wants be able to do tdmd -version=Foo myfile.d (and not use @marler8997's fork of rdmd yet).

#!/bin/bash

DMD="${DMD:-dmd}"
file="$1"
other_flags=()
shift

for arg in "$@" ; do
    if [ -f "$arg" ] ; then
        other_flags+=("$file")
        file="$arg"
    else
        other_flags+=("$arg")
    fi
done

"$DMD" -i "${other_flags[@]}" -run "${file}"

(combine this with alias tdmd = rdmd2 -unittest -main)

@marler8997
Copy link
Contributor Author

Cleaning up old PRs that aren't going anywhere

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
9 participants