rdmd: support --eval <arg> to make it Makefile friendly by Superstar64 · Pull Request #336 · dlang/tools

Superstar64 · 2018-03-19T11:04:28Z

This a patch to make rdmd work for makefiles using a -e flag.

.ONESHELL:
SHELL = /usr/bin/rdmd
.SHELLFLAGS = -e 
hello.txt:
	import std.file;
	write("$@","hello world\n");

note --eval= won't work in SHELLFLAGS because make passes SHELLFLAGS and the code in seperate arguments.

Invoked with: ["/home/superstar64/Desktop/tools/rdmd", "-e", "import std.file;\nwrite(\"hello.txt\",\"hello world\\n\");"]

dlang-bot · 2018-03-19T11:04:30Z

Thanks for your pull request and interest in making D better, @Superstar64! 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

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub fetch digger
dub run digger -- build "master + tools#336"

CyberShadow · 2018-03-19T12:26:09Z

Interesting addition, thanks for the PR!

However, I'm not sure if -e is the best choice for a flag. So far, rdmd has completely avoided single-dash options (including short, single-letter options), and left those to the compiler (DMD). Adding -e here now would mean that should DMD need to add such a flag, it won't be accessible via rdmd.

Is there a restriction that the flag must be single-letter? Why not just allow --eval to work if it's not specified as --eval=code, but also with code as a separate argument?

The following simpler patch works for me:

diff --git a/rdmd.d b/rdmd.d
index 4ca39b3..d547f28 100755
--- a/rdmd.d
+++ b/rdmd.d
@@ -330,7 +330,8 @@ size_t indexOfProgram(string[] args)
     foreach(i, arg; args[1 .. $])
     {
         if (!arg.startsWith('-', '@') &&
-                !arg.endsWith(".obj", ".o", ".lib", ".a", ".def", ".map", ".res"))
+                !arg.endsWith(".obj", ".o", ".lib", ".a", ".def", ".map", ".res") &&
+                args[i /*- 1 + 1*/] != "--eval")
         {
             return i + 1;
         }

Also, tests & documentation would be nice!

wilzbach · 2018-03-19T12:52:55Z

Also, tests & documentation would be nice!

And changelog entry :)

Superstar64 · 2018-03-19T13:46:11Z

Pardon my ignorance, but how do I add documentation and to the change log?

marler8997 · 2018-03-19T13:49:31Z

rdmd.d

        if (!arg.startsWith('-', '@') &&
-                !arg.endsWith(".obj", ".o", ".lib", ".a", ".def", ".map", ".res"))
+                !arg.endsWith(".obj", ".o", ".lib", ".a", ".def", ".map", ".res") &&
+                args[i /*- 1 + 1*/] != "--eval")


This is already checked via !arg.startsWith('-', '@') isn't it?

No, the new snippet allows --eval code. Before it would think code was a file.

oh I see. args[i] != arg...very confusing I must say. I see you put a comment to try to indicate this.

Maybe create a new variable previousArg to make it less confusing?

wilzbach · 2018-03-19T14:21:44Z

Simply add a file to the changelog folder - see e.g. this Readme to get started: https://github.com/dlang/dmd/blob/master/changelog/README.md
(For some reason there doesn't seem to be a changelog folder here, but it's probably due to it being purged on every release)

wilzbach · 2018-03-19T14:23:26Z

posix.mak


-test_rdmd: $(ROOT)/rdmd_test $(RDMD_TEST_EXECUTABLE)
+export RDMD_TEST_EXECUTABLE
+export ROOT


Can we just export these variables when invoking the Makefile to avoid leakage and being explicit?

wilzbach · 2018-03-19T14:26:18Z

(also mind the CI failures)

Superstar64 · 2018-03-19T14:30:41Z

Not completely sure, but I think the reason the test failing is because .SHELLFLAGS is a gnu extension. What should I do about that?

wilzbach · 2018-03-19T14:46:35Z

Travis uses Ubuntu 14.04 which uses GNUmake, but maybe an old version?

Superstar64 · 2018-03-19T14:57:25Z

It appears to be ignoring .ONESHELL and .SHELLFLAGS. Might be make's posix compliant mode?

marler8997 · 2018-03-19T14:59:44Z

changelog/rdmdMakefiles.dd

@@ -0,0 +1,12 @@
+rdmd can now used as a shell in makefiles


rdmd can now be used as a shell in makefiles

Superstar64 · 2018-03-19T15:02:18Z

The only ways I can think of fixing this is to have either break compatibility and make rdmd interpret -c as --eval,have a separate program rdmd_make which intercepts -c, or force gnu extensions for unittesting.

marler8997 · 2018-03-19T15:04:04Z

rdmd.d

 {
    foreach(i, arg; args[1 .. $])
    {
+        //args[i] check for the previous argument


How about this? Seems much less confusing to me:

size_t indexOfProgram(string[] args) { foreach (i; 1 .. args.length) { auto arg = args[i]; if (!arg.startsWith('-', '@') && !arg.endsWith(".obj", ".o", ".lib", ".a", ".def", ".map", ".res") && args[i - 1] != "--eval") { return i; } } return args.length; }

The current code would be better written as

foreach (previousArgIndex, arg; args[1 .. $])

...definitely confusing...

marler8997 · 2018-03-19T15:26:50Z

Not sure what the best solution would be, but another idea would be to create a shell wrapper that looks something like this, i.e.

rdmd_shell:

#!/bin/bash
$(dirname "$(readlink -f "$0")")/rdmd --eval $@

marler8997 · 2018-03-19T15:30:22Z

Yet another idea is to create an alias to rdmd called rdmd_shell. Then in rdmd you could do:

if (args[0].baseName == "rdmd_shell")
{
    // behave as if --eval was given
}

Superstar64 · 2018-03-20T12:58:17Z

Ok i've figured it out, .SHELLFLAGS and .ONESHELL was added in make 3.82 and ubuntu 14.04 uses make 3.81.
https://lists.gnu.org/archive/html/info-gnu/2010-07/msg00023.html
https://packages.ubuntu.com/trusty/make

CyberShadow · 2018-03-24T02:36:25Z

Another way to do the test would be to make rdmd_test create the makefile script and invoke make on it. It already creates the test D programs, so this wouldn't be a large deviation. This also allows making the test optional.

CyberShadow · 2018-03-24T02:33:56Z

changelog/rdmdMakefiles.dd

+.SHELLFLAGS = --eval
+hello.txt:
+	import std.file;
+	write("$@","hello world\n");


No tabs please.

wilzbach · 2018-03-24T12:57:40Z

rdmd_makefile_test.mak

+SHELL = $(RDMD_TEST_EXECUTABLE)
+.SHELLFLAGS = --eval
+$(ROOT)/rdmd_makefile_test.txt:
+	import std.file;


Entire Phobos is auto-imported with --eval ;-)

wilzbach · 2018-03-24T12:58:03Z

changelog/rdmdMakefiles.dd

+SHELL = $(RDMD_TEST_EXECUTABLE)
+.SHELLFLAGS = --eval
+hello.txt:
+    import std.file;


Entire Phobos is auto-imported with --eval ;-)

That import is there because std.file : write conflicts with std.stdio : write. Would you rather it be std.file.write(...)?

wilzbach · 2018-03-24T13:08:17Z

Ok i've figured it out, .SHELLFLAGS and .ONESHELL was added in make 3.82 and ubuntu 14.04 uses make 3.81.

Probably something worthwhile to mentioned in the changelog.

Ideas to workaround this:

switching from Travis to CircleCi
building a newer make from source

Building from source isn't so hard:

curl https://ftp.gnu.org/gnu/make/make-4.2.tar.gz | tar -xz
cd make-4.2
./configure
./build.sh
cd ..
MAKE=$(PWD)/make-4.2/make

Superstar64 · 2018-03-24T13:49:26Z

build make from source

Should there be a check if the version of make running the makefile is least 3.82?
Should make 4.2 be built or make 3.82?
Where should the tar.gz be put? ./generated, tmpDir()?

wilzbach · 2018-03-24T14:02:34Z

Imho it's safe that all other systems running the tool's testsuite use 3.82 or higher, so I would simply put this building code in travis.sh (I doesn't really matter where you put it on Travis, but generated is probably a good choice in case someone ever wants to use this script locally).

Superstar64 · 2018-03-24T15:38:48Z

Sorry it took me so many commits to get travis working

CyberShadow

LGTM. @wilzbach ?

wilzbach

LGTM too! Thanks a lot for the hard work!

wilzbach · 2018-03-25T20:05:36Z

changelog/rdmdMakefiles.dd

+.SHELLFLAGS = --eval
+hello.txt:
+	import std.file;
+	write("$@","hello world\n");


FYI: @CyberShadow we should use tabs here, s.t. users can easily copy/paste this file.

wilzbach · 2018-03-25T20:07:11Z

changelog/rdmdMakefiles.dd

+SHELL = /usr/bin/rdmd
+.SHELLFLAGS = --eval
+hello.txt:
+	import std.file;


We should really resolve this conflict between std.stdio.write and std.file.write.
There's one workaround though: "$@".File("w").writeln("hello world");, but I'm not sure if it's better than the import.

wilzbach · 2018-03-25T20:07:40Z

rdmd.d

        if (!arg.startsWith('-', '@') &&
-                !arg.endsWith(".obj", ".o", ".lib", ".a", ".def", ".map", ".res"))
+                !arg.endsWith(".obj", ".o", ".lib", ".a", ".def", ".map", ".res") &&
+                args[i - 1] != "--eval")


@Superstar64 FYI: I fixed the style here. DStyle is to use whitespace before/after binary operators.

wilzbach · 2018-03-25T20:25:09Z

changelog/rdmdMakefiles.dd

+SHELL = /usr/bin/rdmd
+.SHELLFLAGS = --eval
+hello.txt:
+    import std.file;


We should really resolve this conflict between std.stdio.write and std.file.write.
There's one workaround though: "$@".File("w").writeln("hello world");, but I'm not sure if it's better than the import.

There's also toFile now :)

wilzbach · 2018-03-25T20:25:55Z

changelog/rdmdMakefiles.dd

+.SHELLFLAGS = --eval
+hello.txt:
+    import std.file;
+    write("$@","hello world\n");


FYI: @CyberShadow we should use tabs here, s.t. users can easily copy/paste this file, but DAutoTest complains about it, so I guess we just keep spaces for the sake of simplicity.

Maybe we can get a $(TAB) DDoc macro?

Yes good idea: dlang/dlang.org#2296

Superstar64 · 2018-03-25T21:15:41Z

travis build failed because of a http 502. how do request a rebuild?

wilzbach · 2018-03-25T21:17:34Z

You just did so. FWIW you might be interested in dlang/installer#273 which would avoid this error, but it's in the queue for a long time already with Martin being so busy :/

Superstar64 · 2018-03-25T21:19:41Z

Wait isn't the commit message misleading?

Superstar64 · 2018-03-25T21:23:50Z

Thank you

wilzbach · 2018-03-25T21:24:42Z

Yeah, I just squashed to your initial one. Thanks!
I changed the message + title (though instead of asking you could have done it yourself - would save the roundtrip).

wilzbach · 2018-03-26T01:13:36Z

(added $(TAB))

Superstar64 requested review from CyberShadow, andralex and wilzbach as code owners March 19, 2018 11:04

marler8997 reviewed Mar 19, 2018

View reviewed changes

wilzbach reviewed Mar 19, 2018

View reviewed changes

marler8997 reviewed Mar 19, 2018

View reviewed changes

marler8997 mentioned this pull request Mar 20, 2018

Simplify interface to rdmd_test and make it more robust #331

Closed

CyberShadow reviewed Mar 24, 2018

View reviewed changes

wilzbach reviewed Mar 24, 2018

View reviewed changes

CyberShadow approved these changes Mar 25, 2018

View reviewed changes

wilzbach approved these changes Mar 25, 2018

View reviewed changes

wilzbach added the auto-merge label Mar 25, 2018

wilzbach removed the auto-merge label Mar 25, 2018

wilzbach mentioned this pull request Mar 25, 2018

Add $(TAB) macro dlang/dlang.org#2296

Merged

wilzbach changed the title ~~added -e to rdmd to make it makefile friendly~~ rdmd: support --eval <arg> to make it Makefile friendly Mar 25, 2018

wilzbach added the auto-merge label Mar 25, 2018

rdmd: support --eval <arg> to make it Makefile friendly

e88c44d

dlang-bot removed the auto-merge label Mar 26, 2018

wilzbach added the auto-merge label Mar 26, 2018

dlang-bot merged commit 9064366 into dlang:master Mar 26, 2018

Superstar64 deleted the makefile_compat_rdmd branch March 26, 2018 02:34

Uh oh!

Comments

Conversation

Superstar64 commented Mar 19, 2018

Uh oh!

dlang-bot commented Mar 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Bugzilla references

Testing this PR locally

Uh oh!

CyberShadow commented Mar 19, 2018

Uh oh!

wilzbach commented Mar 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Superstar64 commented Mar 19, 2018

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!

wilzbach commented Mar 19, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wilzbach commented Mar 19, 2018

Uh oh!

Superstar64 commented Mar 19, 2018

Uh oh!

wilzbach commented Mar 19, 2018

Uh oh!

Superstar64 commented Mar 19, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Superstar64 commented Mar 19, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marler8997 Mar 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marler8997 commented Mar 19, 2018

Uh oh!

marler8997 commented Mar 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Superstar64 commented Mar 20, 2018

Uh oh!

CyberShadow commented Mar 24, 2018

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!

wilzbach commented Mar 24, 2018

Uh oh!

Superstar64 commented Mar 24, 2018

Uh oh!

wilzbach commented Mar 24, 2018

Uh oh!

Superstar64 commented Mar 24, 2018

Uh oh!

CyberShadow left a comment

Choose a reason for hiding this comment

Uh oh!

wilzbach 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!

dlang-bot commented Mar 19, 2018 •

edited

Loading

wilzbach commented Mar 19, 2018 •

edited

Loading

marler8997 Mar 19, 2018 •

edited

Loading

marler8997 commented Mar 19, 2018 •

edited

Loading