Parse all command line options in repl.c #9482

jakebolewski · 2014-12-29T03:37:15Z

Addresses #9384. Currently the options that are not in jl_compileropts_t are stored into a new jl_options_t struct. This struct is then loaded when processing the command line options in base/client.jl. This PR adds some sanity check tests for command line arguments as well.

I think it would be better to rename jl_compileropts_t and just have one option struct for all command line arguments (and api to query the arguments from Julia).

I've deprecated --no-history-file and --no-startup flags in favor of --history-file={yes|no} and --startup-file={yes|no} so all command line options have a similar structure.

tkelman · 2014-12-29T03:47:16Z

While you're at it, documentation of these is duplicated across several different places, doc/manual/getting-started.rst and doc/man/julia.1 and possibly others, all of which have fallen out of date.

vtjnash · 2014-12-29T04:03:44Z

this seems good. +1

one thing to be aware of is that some of the arguments, such as -F, -E and -n, previously had immediate effect when encountered, and thus could be repeated

jakebolewski · 2014-12-29T04:10:21Z

@vtjnash good point, I couldn't replicate all previous behavior with how command line options were processed so it would be useful if others test this out to see if I broke something.

samoconnor · 2015-01-03T07:57:53Z

Nice one.
It was confusing for me to try to find how the "-e" option was handled because it was not in repl.c.
See #9468 and specifically: https://github.com/samoconnor/julia/blame/doc_notes_branch/doc/devdocs/init.rst#L22

jakebolewski · 2015-01-05T03:51:41Z

Rebased and squashed. I consolidated all options into a single struct as talked about. The old jl_compileropts struct was exported so I'm unsure if we consider this part of the "public" C API. On the julia side, the option struct now lives in base/options.jl. I also added more tests and updated the documentation.

jakebolewski · 2015-01-05T20:09:26Z

The tests on Windows should pass now.

tkelman · 2015-01-05T20:12:17Z

doc/manual/getting-started.rst


+    -O, --optimize
+                              Run time-intensive code optimizations


why the separate line here?

I don't know, maybe I found it easier to read at the time?

tkelman · 2015-01-06T23:09:56Z

Needs a rebase already?

tkelman · 2015-01-11T23:11:16Z

ui/repl.c


 void parse_opts(int *argcp, char ***argvp)
 {
-    static char* shortopts = "+H:hJ:C:O";
+    enum { machinefile = 300,
+	       color,


no tabs whenever you rebase this

Ah, thanks. I have to change my Vim defaults.

note that #9732 might get merged before this and touches the same code

Sorry about that, @jakebolewski.

jakebolewski · 2015-01-13T23:21:06Z

Ok, I rebased just rebased. It needs some review as it touches a lot of code and I'm unsure if shoving everything into one struct is the cleanest way to do this.

ihnorton · 2015-01-14T06:06:05Z

base/client.jl

+    global process_options
+    function process_options(opts::JLOptions, args::Vector{UTF8String})
+        if !isempty(args) && (arg = first(args); arg[1] == '-' && in(arg, reqarg))
+            println(STDERR, "julia: option `$arg` is missing an argument")


couldn't we do this by checking the return value of getopt_long? http://man7.org/linux/man-pages/man3/getopt.3.html#RETURN_VALUE

That would be possible if the long option struct array was an exported global.

ihnorton · 2015-01-14T06:13:45Z

aesthetics (ignore, more or less)

looks_like(deity_obj) && smells_like(deity_obj) == true
at very least, having something called eval in there feels weird

other than my one inline comment, the actual code LGTM. lots of repetition but i guess that is unavoidable here. it would be nice if we could store all of the text-based/non-hot longopts in a dictionary, but i guess that would be harder to use (or not available at all when needed) from some parts of the C code.

ihnorton · 2015-02-12T03:32:32Z

Bump

tkelman · 2015-02-12T03:41:44Z

#9468, #9450, or both?

jakebolewski · 2015-02-12T03:44:57Z

9450, he has been rebasing it too many times I don't want to add to his pain :-)

JeffBezanson · 2015-02-16T22:35:41Z

9450 merged.

StefanKarpinski · 2015-02-17T20:34:02Z

See also #10226 – argument parsing should stop at --.

jakebolewski · 2015-02-17T20:54:26Z

The c library that we use for unix opt parsing, ui/getopt.c would have to be changed to support this.

StefanKarpinski · 2015-02-17T20:56:08Z

How feasible is just moving all of the option parsing into Julia? Most of the REPL is in Julia these days so it seems like it shouldn't be too crazy to do that.

jakebolewski · 2015-02-17T21:01:02Z

I don't think too feasible at the moment. How do you turn off boundschecking for generated julia code that is being executed to parse the option to turn off boundschecking?

tkelman · 2015-02-17T21:02:46Z

Moving things to Julia is the opposite direction from what this PR does. And there's a bootstrapping problem for the flags that control compiler options, which is a good chunk of them.

Note that we use the system version of getopt everywhere except in the (currently broken) MSVC build.

samoconnor · 2015-02-17T22:55:00Z

@tkelman says that system getopt is used by default.
Double dash end of args should already be handled by the system getopt:

Linux: The special argument "--" forces an end of option-scanning regardless of the scanning mode
BSD: The interpretation of options in the argument list may be cancelled by
the option `--' (double dash) which causes getopt() to signal the end of
argument processing and return -1.

Is there an overall policy for how cross-platform issue should be handled in Julia?

e.g. my usual rules for a non-GUI cross-platform system would be:

Use POSIX-ish APIs
Use GNU extensions, BSD extensions, etc wherever these have been adopted by the important platforms (i.e. Linux and OSX)
Disallow #ifdef WIN32 in the main codebase
Support windows by by linking to a compatibility library that provides whatever POSIX-ish functions are missing on windows (e.g. ui/getopt.c and there are lots of implementation of posix functions for windows out there...)
Allow building against cygwin as an alternative.

The alternatives are:

Code full of #ifdef WIN32
Only supporting the smallest common subset of OS-librarty features
Building yet-another-cross-platform-API-wrapper-layer-library-thingamabob.

tkelman · 2015-02-17T22:57:00Z

@samoconnor everything you describe is what we use libuv for...

samoconnor · 2015-02-17T23:56:37Z

True, libuv is used (and seems like a decent choice) for network and IO stuff, but there is still a lot of #ifdef OS_WINDOWS elsewhere.

find julia/src/ -name '*.c' | xargs grep -l OS_WINDOWS | wc -l
26

tkelman · 2015-02-18T00:12:44Z

Well going out of your way to have fewer ifdefs if you don't deem Windows an "important platform" is an option for some projects. Cygwin is not an option for Julia since Cygwin doesn't implement any asynchronous IO required by libuv.

For things that libuv doesn't cover, like a different set of headers being available, libc functions like strtod having different functionality, handling of paths and executables and shared libraries and exception handling and debug info and calling conventions (and and and) being different, etc, they're handled on a case-by-case basis. I don't think there's one unifying policy, other than things mostly work now, and we like making them work better where possible. Refactorings that clean things up are also welcome when they don't harm existing functionality.

samoconnor · 2015-02-18T01:13:28Z

@tkelman ... that all sounds reasonable.
But, I think something being an "important platform" is an emotive way to think about it.
I think you can have windows be an "important platform" while simultaneously saying "libuv + POSIX(-ish) is the primary OS API layer, and windows specific implementations are hidden behind that". The former is a matter of marketing and customer support, the latter is a matter of architectural discipline and maintainability.

I think that it's important to have clear rules on which layers are allowed to talk to which other layers.
One man's quick fix for platform-xyz becomes another mans refactoring nightmare later on...

(apologies to @jakebolewski for going so far off topic on your P.R.)

tkelman · 2015-02-18T01:27:47Z

We'd actually rather remove our posix assumptions as much as possible. Ideally libuv should stand between us and either posix or win32 for as much as it can. There are plenty of places that it can't do the job completely though, and llvm doesn't abstract away the OS to the extent that would allow us to clean up codegen, debuginfo, ffi, etc. The new ORC JIT API's that are being worked on in 3.7 might be nicer in this respect, I'm not sure?

I did some quick comparisons and we have fewer Windows ifdefs than cmake or CPython, but evidently a bit more than either LLVM or luajit (or I was just searching for the wrong defines in those projects). Julia's C core is pretty small by language implementation standards, but it tends to be grouped by functionality rather than having centralized platform differences consolidated into a handful of headers or source files.

vtjnash · 2015-02-18T03:42:13Z

I believe that LLVM separates a lot of it's host specific code into separate files (including autogenerated tblgen #include files), so that fewer ifdef's can cover more code.

I don't really care how many ifdef's are needed for the C code. My goal is to largely eliminate the need for them in user code inside of Julia, by providing a user interface that can be consistently implemented across platform. this effort is greatly assisted by libuv, since it provides a consistent interface for dealing with a variety of platform-specific issues (generally related to I/O).

- consolidate all compiler / cmdline options into jl_options_t struct in julia.h - add options.jl to base/ with an immutable type JLOptions that reflects the jl_options_t struct - add --procs=<n> command line flag (equivalent to -p <n>) - add --history-file={yes|no} and --startup-file={yes|no} cmdline opts - --worker command line arguments changed to --worker, --worker=default, or --worker=custom - deprecate -f, -F, --no-startup, --no-history-file cmdline flags - add tests for cmdline arguments in test/cmdlineargs.jl - modify test/Makefile and tests to use long command line option, add command line argument tests to runtests.jl - update relevant docs in the manual and manpages

jakebolewski · 2015-02-21T18:44:20Z

Ok, rebased.

@amitmurthy I had to change --worker custom to --worker=custom (--worker is the same as --worker=default) reflecting recent changes to the cluster manager interface.

Parse all command line options in repl.c

amitmurthy · 2015-02-22T03:21:29Z

OK. Nice work on the cleanup.

JeffBezanson · 2015-02-23T01:12:08Z

base/client.jl

+                startup = false
+            end
+            # load ~/.julia_history file
+            no_history_file = bool(opts.historyfile)


Should there be a ! here?

I switched this to history_file=true/false which follows other changes in this PR.

jakebolewski force-pushed the jcb/fixcopts branch from 586b9e1 to eb575ba Compare December 29, 2014 03:39

tkelman mentioned this pull request Dec 30, 2014

Add --noinline startup option #9354

Merged

jakebolewski force-pushed the jcb/fixcopts branch from cd1ac6b to f32430e Compare January 2, 2015 20:13

jakebolewski force-pushed the jcb/fixcopts branch 3 times, most recently from 59a8448 to 8301aa6 Compare January 5, 2015 03:44

jakebolewski changed the title ~~WIP: Parse all command line options in repl.c~~ Parse all command line options in repl.c Jan 5, 2015

jakebolewski force-pushed the jcb/fixcopts branch from 9ac4e71 to 8cfc435 Compare January 5, 2015 20:05

tkelman reviewed Jan 5, 2015
View reviewed changes

jakebolewski mentioned this pull request Jan 6, 2015

--inline flag not documented in man page #9641

Closed

jakebolewski force-pushed the jcb/fixcopts branch from 8cfc435 to ef382d0 Compare January 6, 2015 19:59

This was referenced Jan 6, 2015

Implement @fastmath #9406

Merged

Minor code and style improvements in ui/repl.c #9732

Merged

tkelman reviewed Jan 11, 2015
View reviewed changes

jakebolewski force-pushed the jcb/fixcopts branch 3 times, most recently from 072b017 to 31d683d Compare January 13, 2015 23:18

ihnorton reviewed Jan 14, 2015
View reviewed changes

jiahao mentioned this pull request Feb 2, 2015

Remove --int-literals option #9597

Merged

jakebolewski mentioned this pull request Feb 17, 2015

ARGS regression: options should not end up in ARGS #10227

Closed

ihnorton mentioned this pull request Feb 18, 2015

Process Name Change? #9957

Closed

jakebolewski force-pushed the jcb/fixcopts branch from 31d683d to a7dad31 Compare February 20, 2015 23:11

jakebolewski force-pushed the jcb/fixcopts branch from 3a2c582 to 25b89e8 Compare February 21, 2015 18:38

jakebolewski added a commit that referenced this pull request Feb 21, 2015

Merge pull request #9482 from JuliaLang/jcb/fixcopts

6051af5

Parse all command line options in repl.c

jakebolewski merged commit 6051af5 into master Feb 21, 2015

jakebolewski deleted the jcb/fixcopts branch February 21, 2015 21:09

jakebolewski mentioned this pull request Feb 21, 2015

Command line options are currently broken #9384

Closed

JeffBezanson reviewed Feb 23, 2015
View reviewed changes

tkelman mentioned this pull request May 5, 2015

Travis failing on package tests with 0.3.8 release #11132

Closed

tkf mentioned this pull request Sep 27, 2018

Make InterruptException capturable in a program #29383

Closed

Parse all command line options in repl.c #9482

Parse all command line options in repl.c #9482

Conversation

jakebolewski commented Dec 29, 2014

tkelman commented Dec 29, 2014

vtjnash commented Dec 29, 2014

jakebolewski commented Dec 29, 2014

samoconnor commented Jan 3, 2015

jakebolewski commented Jan 5, 2015

jakebolewski commented Jan 5, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tkelman commented Jan 6, 2015

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

jakebolewski commented Jan 13, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ihnorton commented Jan 14, 2015

ihnorton commented Feb 12, 2015

tkelman commented Feb 12, 2015

jakebolewski commented Feb 12, 2015

JeffBezanson commented Feb 16, 2015

StefanKarpinski commented Feb 17, 2015

jakebolewski commented Feb 17, 2015

StefanKarpinski commented Feb 17, 2015

jakebolewski commented Feb 17, 2015

tkelman commented Feb 17, 2015

samoconnor commented Feb 17, 2015

tkelman commented Feb 17, 2015

samoconnor commented Feb 17, 2015

tkelman commented Feb 18, 2015

samoconnor commented Feb 18, 2015

tkelman commented Feb 18, 2015

vtjnash commented Feb 18, 2015

jakebolewski commented Feb 21, 2015

amitmurthy commented Feb 22, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment