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

[sbt 1.0] Rename early command to early(command) #2741

Merged
merged 3 commits into from
Sep 15, 2016

Conversation

eed3si9n
Copy link
Member

@eed3si9n eed3si9n commented Sep 15, 2016

Fixes #2734, Ref #1041

e93c445 added a feature called early command, which uses -- as a prefix to denote some commands that runs ahead of session loading. While the feature might be useful especially for logging, -- is too useful just for this purpose.

In addition, this adds log level commands with single -, such as -error to treat them as early commands.

/review @dwijnand, @Duhemm

Fixes sbt#2734, Ref sbt#1041
e93c445 added a feature called early
command, which uses `--` as a prefix to denote some commands that runs
ahead of session loading. While the feature might be useful especially
for logging, `--` is too useful just for this purpose.
Copy link
Member

@dwijnand dwijnand left a comment

Choose a reason for hiding this comment

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

Why not keep these as --debug/--warn/--error? -debug/-warn/-error is less UNIX-y that that.

This way we remove -- from making everything early to just special cases.

@eed3si9n
Copy link
Member Author

eed3si9n commented Sep 15, 2016

It's true that general Unix convention would be to use double hyphen for long options, but java, scala, and sbt have been using single hyphen for options. See https://github.com/sbt/sbt-launcher-package/blob/master/src/universal/bin/sbt

  -d | -debug        set sbt log level to debug

@dwijnand
Copy link
Member

That is true.. But what's wrong with keeping it as --error? It conveys the right meaning, it doesn't clash with anything, and most importantly it's backwards compatible.

@eed3si9n
Copy link
Member Author

eed3si9n commented Sep 15, 2016

I'd keep it for 0.13 for compat but I don't really see strong reason to keep it for 1.0.
early(command) is the generic handler, and -error would be a new option that's consistent with every other options. The reason for removal is the same reason for removing <<=, which is reducing the surface area.

@dwijnand
Copy link
Member

dwijnand commented Sep 15, 2016

Ok. I think it's much better if we don't do these breaking changes in point releases, but instead documented in the upgrade/migration notes. Thanks

@dwijnand
Copy link
Member

For clarity, I think we should add notes here.

eed3si9n added a commit to eed3si9n/sbt that referenced this pull request Sep 15, 2016
Backports sbt#2741, Fixes sbt#1041

e93c445 added a feature called early command, which uses `--` as a
prefix to denote some commands that runs ahead of session loading.
While the feature might be useful especially for logging, `--` is too
useful just for this purpose.

In addition, this adds log level commands with single `-`, such as
-error to treat them as early commands; and keeps `--` variant for log
level for backward compatibility.
@eed3si9n
Copy link
Member Author

Added migration notes - 9783ab1

@retronym
Copy link
Member

retronym commented Apr 10, 2018

Over in scala/scala#6256, we found that:

$ sbt '--warn' 'eval ???'; echo $?
[warn] The `-` command is deprecated in favor of `onFailure` and will be removed in a later version
[error] scala.NotImplementedError: an implementation is missing
[error] 	at scala.Predef$.$qmark$qmark$qmark(Predef.scala:284)
[error] 	at $b7f279a4d8e441649156$.$sbtdef(<eval>:1)
[error] 	at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
[error] 	at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
[error] 	at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
[error] 	at java.lang.reflect.Method.invoke(Method.java:498)
[error] 	at sbt.compiler.Eval$.getValue(Eval.scala:551)
[error] 	at sbt.compiler.Eval.$anonfun$eval$1(Eval.scala:121)
[error] 	at sbt.BuiltinCommands$.$anonfun$loadedEval$1(Main.scala:383)
[error] 	at sbt.internal.util.ManagedLogger.log(ManagedLogger.scala:24)
[error] 	at sbt.util.Logger.info(Logger.scala:22)
[error] 	at sbt.BuiltinCommands$.loadedEval(Main.scala:383)
[error] 	at sbt.BuiltinCommands$.$anonfun$eval$1(Main.scala:374)
[error] 	at sbt.Command$.$anonfun$apply1$2(Command.scala:146)
[error] 	at sbt.MainLoop$.processCommand(MainLoop.scala:154)
[error] 	at sbt.MainLoop$.$anonfun$next$2(MainLoop.scala:137)
[error] 	at sbt.State$$anon$1.runCmd$1(State.scala:242)
[error] 	at sbt.State$$anon$1.process(State.scala:248)
[error] 	at sbt.MainLoop$.$anonfun$next$1(MainLoop.scala:137)
[error] 	at sbt.internal.util.ErrorHandling$.wideConvert(ErrorHandling.scala:16)
[error] 	at sbt.MainLoop$.next(MainLoop.scala:137)
[error] 	at sbt.MainLoop$.run(MainLoop.scala:130)
[error] 	at sbt.MainLoop$.$anonfun$runWithNewLog$1(MainLoop.scala:108)
[error] 	at sbt.io.Using.apply(Using.scala:22)
[error] 	at sbt.MainLoop$.runWithNewLog(MainLoop.scala:102)
[error] 	at sbt.MainLoop$.runAndClearLast(MainLoop.scala:58)
[error] 	at sbt.MainLoop$.runLoggedLoop(MainLoop.scala:43)
[error] 	at sbt.MainLoop$.runLogged(MainLoop.scala:35)
[error] 	at sbt.StandardMain$.runManaged(Main.scala:113)
[error] 	at sbt.xMain.run(Main.scala:76)
[error] 	at xsbt.boot.Launch$$anonfun$run$1.apply(Launch.scala:109)
[error] 	at xsbt.boot.Launch$.withContextLoader(Launch.scala:128)
[error] 	at xsbt.boot.Launch$.run(Launch.scala:109)
[error] 	at xsbt.boot.Launch$$anonfun$apply$1.apply(Launch.scala:35)
[error] 	at xsbt.boot.Launch$.launch(Launch.scala:117)
[error] 	at xsbt.boot.Launch$.apply(Launch.scala:18)
[error] 	at xsbt.boot.Boot$.runImpl(Boot.scala:41)
[error] 	at xsbt.boot.Boot$.main(Boot.scala:17)
[error] 	at xsbt.boot.Boot.main(Boot.scala)
[error] scala.NotImplementedError: an implementation is missing
[error] Use 'last' for the full log.
0

gives a zero error code, because this is now the same as sbt 'onFailure -warn' ....

I'll assume that this was unintentional. This is quite dangerous, as sbt-extra lists -w in its help which it translates to --warn. Both sbt-extras and the official sbt launcher passes through --warn, which is was our scripts have been using.

It is quite easy to miss or ignore the warning, and then run into the trap of having build scripts pass when a test fails.

Thoughts? I'd recommend adding --warn etc as aliases, possibly deprecated.

@eed3si9n
Copy link
Member Author

onFailure kicking in was unintentional. Maybe it's time to remove - command too.
For compat (and also for JEP 293), I'd be happy to bring back --warn as special case of early(warn).

@eed3si9n
Copy link
Member Author

Opened an issue for this #4088

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

Successfully merging this pull request may close these issues.

3 participants