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

Allow first character of OCAMLPARAM to specify an alternative separator #1166

Merged
merged 1 commit into from Sep 29, 2017

Conversation

Projects
None yet
@lefessan
Copy link
Contributor

lefessan commented May 10, 2017

In the current implementation of OCAMLPARAM, there is no way to escape a comma in the arguments (for example, if you want to pass an option to the linker in ccopts). This PR provides such a way, by allowing the user to specify an alternative separator, instead of comma, as first character of OCAMLPARAM. The alternative separator must be in ":|; ".

For example, OCAMLPARAM='x=1,_,y=1' can also be written OCAMLPARAM='|x=1|_|y=1'.

A small bug fix also is included, by adding a ccopt option, in addition to ccopts.

@diml

This comment has been minimized.

Copy link
Member

diml commented May 10, 2017

Personally, I find OCAMLPARAM already painful to use. If you are in a shell script and want to extend it you have to first check whether it's empty or not. Now you'll also have to check what syntax it's using.

Why not simply use one environment variable per setting, like what opam does?

@lefessan

This comment has been minimized.

Copy link
Contributor Author

lefessan commented May 10, 2017

@diml I would be curious to know when you need to do that. The intention for OCAMLPARAM was that it should only be used to manually modify a build, so cascading effects should not appear.

Anyway, this PR is not about replacing OCAMLPARAM by another mechanism, but to fix the current limitation that prevents commas in arguments. It should be done even if we decide to think about something else on the long term.

@diml

This comment has been minimized.

Copy link
Member

diml commented May 10, 2017

@diml I would be curious to know when you need to do that. The intention for OCAMLPARAM was that it should only be used to manually modify a build, so cascading effects should not appear.

I do that in jbuilder, to force the compiler to print with colors even though the output is redirected. jbuilder then strips colors itself depending on its own output.

@dbuenzli

This comment has been minimized.

Copy link
Contributor

dbuenzli commented May 10, 2017

@diml there's now #1098 and/or you could simply use -color always

@diml

This comment has been minimized.

Copy link
Member

diml commented May 10, 2017

Indeed, this should simplify things

@lefessan

This comment has been minimized.

Copy link
Contributor Author

lefessan commented May 15, 2017

If there is no opposition, I will commit this feature by the end of the week.

@shindere

This comment has been minimized.

Copy link
Contributor

shindere commented May 16, 2017

@lefessan

This comment has been minimized.

Copy link
Contributor Author

lefessan commented May 16, 2017

Thanks Sebastien for your comment. I updated the manual, the description of OCAMLPARAM was pretty vague, so I made it a bit more explicit, still without specifying the names of the arguments.

@shindere

This comment has been minimized.

Copy link
Contributor

shindere commented May 16, 2017

@Octachron

This comment has been minimized.

Copy link
Contributor

Octachron commented May 16, 2017

@shindere Concerning the documentation of OCAMLPARAM, the documentation itself is quite recent cf. mantis:7007. Another point is that OCAMLRUNPARAM is indeed limited to one-letter parameters (see byterun/startup_aux), contrarily to OCAMLPARAM.

@rneswold

This comment has been minimized.

Copy link
Contributor

rneswold commented May 16, 2017

@shindere :

Regarding your text, I'd say "interpreted" or "taken into account" rather than inserted / executed. Perhaps our english native speaker friends can comment on this.

I agree with your recommendation: I'd replace "inserted" with "interpreted". For "executed", I'd lean more towards "processed". Or maybe the sentence fragment:

means that "a=x" should be executed before parsing the arguments, and "b=y" after.

could be rewritten as

means that "a=x" should be parsed before the command line, and "b=y" after.

@shindere

This comment has been minimized.

Copy link
Contributor

shindere commented May 17, 2017

@SkySkimmer

This comment has been minimized.

Copy link

SkySkimmer commented May 20, 2017

Why not provide a way to escape commas instead?

@lefessan

This comment has been minimized.

Copy link
Contributor Author

lefessan commented May 22, 2017

@SkySkimmer For two reasons:

  • It is quite standard in the Unix world to use the first char as a separator;
  • Escaping is a complex mechanism, especially if you are operating directly from the shell. Also it creates other questions: how do you escape ? Double comma or backslash comma, in which case you also need to escape backslash...
@shindere

This comment has been minimized.

Copy link
Contributor

shindere commented May 22, 2017

@lefessan

This comment has been minimized.

Copy link
Contributor Author

lefessan commented May 29, 2017

Fabrice Le Fessant (2017/05/22 00:17 -0700):

  • It is quite standard in the Unix world to use the first char as a
    separator;
    Ah? Which examples do you have in mind, actually?

For example, sed uses any character following the s command as the separator. In LaTeX, \verb uses also the next character as separator. Although I don't have other examples in mind, I think it is a common solution to avoid escaping.

@alainfrisch

This comment has been minimized.

Copy link
Contributor

alainfrisch commented Jul 10, 2017

Why do you restrict to a fixed set of separator? Any character which is not a letter and not '_' would work, no?

@lefessan

This comment has been minimized.

Copy link
Contributor Author

lefessan commented Jul 10, 2017

For backward compatibility: we want the separator to be something that was not used before. Indeed, the set would be bigger than now, probably anything except letters, '_' and '='.

@damiendoligez
Copy link
Member

damiendoligez left a comment

You should add a Changes entry.

else
(* allow first char to specify an alternative separator in ":|; " *)
match s.[0] with
| ( ':' | '|' | ';' | ' ' ) as c ->

This comment has been minimized.

@damiendoligez

damiendoligez Jul 18, 2017

Member

You should also allow , here.

let rec iter is_after args before after =
match args with
[] ->
if not is_after then
raise (SyntaxError "no '_' separator found")
else
(List.rev before, List.rev after)
(List.rev before, List.rev after)
| "" :: tail -> iter is_after tail before after

This comment has been minimized.

@damiendoligez

damiendoligez Jul 18, 2017

Member

Note: you are now allowing empty elements in the list.

This comment has been minimized.

@lefessan

lefessan Sep 27, 2017

Author Contributor

Yes, it was actually my intent.

| "ccopts" ->
| "ccopt"
| "ccopts"
->

This comment has been minimized.

@damiendoligez

damiendoligez Jul 18, 2017

Member

Looks like an unrelated change that should be in another PR.

This comment has been minimized.

@lefessan

lefessan Sep 27, 2017

Author Contributor

Is it really necessary to open a new PR ? This change is only to be consistent with the names of arguments.

of "name=value" pairs. A "_" is used to specify the position of
the command line arguments, i.e. "a=x,_,b=y" means that "a=x" should be
executed before parsing the arguments, and "b=y" after. Finally,
an alternative separator than the comma can be specified as the

This comment has been minimized.

@damiendoligez

damiendoligez Jul 18, 2017

Member

I don't think this is idiomatic English. Just drop "than the comma".

the command line arguments, i.e. "a=x,_,b=y" means that "a=x" should be
executed before parsing the arguments, and "b=y" after. Finally,
an alternative separator than the comma can be specified as the
first character of the string, within the set ":|; ".

This comment has been minimized.

@damiendoligez

damiendoligez Jul 18, 2017

Member

Add , here too.

of "name=value" pairs. A "_" is used to specify the position of
the command line arguments, i.e. "a=x,_,b=y" means that "a=x" should be
executed before parsing the arguments, and "b=y" after. Finally,
an alternative separator than the comma can be specified as the

This comment has been minimized.

@damiendoligez

damiendoligez Jul 18, 2017

Member

See below.

@mshinwell

This comment has been minimized.

Copy link
Contributor

mshinwell commented Aug 9, 2017

@lefessan Could you deal with @damiendoligez 's comments so we can finish this off?

@xavierleroy

This comment has been minimized.

Copy link
Contributor

xavierleroy commented Sep 23, 2017

Requested changes have not been performed. Plus, I'd rather have proper escapes rather than this \verb-like trick. Shall we finish this PR or close it?

@damiendoligez damiendoligez modified the milestones: 4.06.0, 4.07-or-later Sep 25, 2017

@lefessan lefessan force-pushed the lefessan:2017-05-10-compenv branch from 696b3f6 to 2c413bb Sep 27, 2017

@lefessan

This comment has been minimized.

Copy link
Contributor Author

lefessan commented Sep 27, 2017

Requested changes performed.

@lefessan

This comment has been minimized.

Copy link
Contributor Author

lefessan commented Sep 28, 2017

Looks like Appveyor is not working, again

@damiendoligez
Copy link
Member

damiendoligez left a comment

Good to merge after one small fix.

Changes Outdated
@@ -14,6 +14,10 @@ be mentioned in the 4.06 section below instead of here.)

### Compiler user-interface and warnings:

- GPR#1166: In OCAMLPARAM, an alternative separator can be specified as
first character (instead of comma) in the set ":|; "

This comment has been minimized.

@damiendoligez

damiendoligez Sep 28, 2017

Member

Add the comma here too, then we're good to merge.

Allow first character of OCAMLPARAM to specify an alternative separator
The alternative separator must be in ":|; ,".

@lefessan lefessan force-pushed the lefessan:2017-05-10-compenv branch from 2c413bb to 190375a Sep 28, 2017

@lefessan

This comment has been minimized.

Copy link
Contributor Author

lefessan commented Sep 28, 2017

Small fix done, rebased in one commit.

@damiendoligez damiendoligez merged commit f20a0a0 into ocaml:trunk Sep 29, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@damiendoligez

This comment has been minimized.

Copy link
Member

damiendoligez commented Sep 29, 2017

Merged into trunk. Do not cherry-pick to 4.06.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.