com-lihaoyi / mill

Mill is a fast JVM build tool that supports Java and Scala. 2-3x faster than Gradle and 5-10x faster than Maven for common workflows, Mill aims to make your project’s build process performant, maintainable, and flexible
https://mill-build.org/
MIT License
2.04k stars 331 forks source link

`-i` and `--no-server` gets ignored with no error when passed as the second option (200USD Bounty) #2869

Closed Gedochao closed 1 month ago

Gedochao commented 10 months ago

From the maintainer Li Haoyi: I'm putting a 200USD bounty on this issue, payable by bank transfer on a merged PR fixing this. -i should raise an error when not passed as the first argument, because it can only take effect when it is first, and passing it as a later argument is a easy footgun that we should give a good error message to help people avoid


./mill --help
# (...)
# -i --interactive                  Run Mill in interactive mode, suitable for opening REPLs and
#                                     taking user input. This implies --no-server and no mill server
#                                     will be used. Must be the first argument.
# (...)
# --no-server                       Run Mill in single-process mode. In this mode, no Mill server
#                                     will be started or used. Must be the first argument.

Both -i and --no-server options require to be the first argument, but no error seems to be produced if they're passed elsewhere.

▶ ./mill --disable-callgraph-invalidation --no-server __.fix
# (standard build logs, no errors or warnings, --no-server gets ignored)
▶ ./mill --disable-callgraph-invalidation -i __.fix
# (standard build logs, no errors or warnings, -i gets ignored)

This caused my team a major headache when --disable-callgraph-invalidation has been added to the Scala CLI mill script as the first argument (attempted workaround for #2844), with -i sneakily being ignored everywhere as a result, with no failures.

I'd at the very least expect a warning, and preferably just an error.

lefou commented 10 months ago

This is strange. We had such a warning in 0.10. I thought we removed this requirement (to have them in first position) as we now have a single entry point to Mill (so we can parse the full cmdline), and probably have removed the warning along this way. We need to have a deeper look...

Gedochao commented 10 months ago

Hm... changing -i back to the first argument spot seemed to fix stuff for our builds, so something seems to still be going on there. I mean, our CI was then breaking elsewhere, but that's a separate thing 😅

Also, if the first argument spot ends up no longer being enforced for these options, it'd be helpful to remove the corresponding sentence from the --help description.

lefou commented 10 months ago

I completely agree. I'll mark this issue a bug once I have a better idea what's going on.

lefou commented 10 months ago

Looks like the requirement to have it in first position is still there.

https://github.com/com-lihaoyi/mill/blob/c12d0245840a9a521c6e65f99b846ac96939ca10/main/client/src/mill/main/client/MillClientMain.java#L62

So I was wrong. We need to fix the missing warning. Thanks for reporting.

lefou commented 10 months ago

Hm, we have it here: https://github.com/com-lihaoyi/mill/blob/c12d0245840a9a521c6e65f99b846ac96939ca10/runner/src/mill/runner/MillMain.scala#L134-L136

Gedochao commented 10 months ago

Hm. Then it seems it just isn't being printed for some reason...

lefou commented 10 months ago

Mill should stop right away, when this branch in entered. So I think the condition is no longer true. Most likely the streams.in == DummyInputStream doesn't hold any longer.

lefou commented 10 months ago

I think Mill learned to use the system IN even when in server mode, so we can't no longer detect if if we run in a server or not by checking for a DummyInputStream.

alastor1729 commented 2 months ago

Hi @Gedochao / @lefou,

I talked with @lihaoyi this morning; Y'all can assign this to me 🙂

Sincerely, R.W.

lihaoyi commented 2 months ago

@alastor1729 I'm happy to help if you need pointers. Feel free to ask here, or we can schedule a call or something if you want to discuss it synchronouslhy

alastor1729 commented 2 months ago

Hi @lihaoyi, I just sent you a "30 mins Google Meeting call" for us to discuss Issue synchronously && ask for pointers:

lefou commented 1 month ago

Any news on that topic? Did you decide on any implementation detail?

If not, I see two options, and it's unclear to me, what the envisioned goal of this ticket is

  1. ensure we show a proper warning in all cases where args are used in non-first position
  2. make these args work in all positions, which is more difficult, since we parse / detect these in a shell script (the prepended script in the binary assembly) and if Mill isn't invoked through that script, we also parse it from Java (which starts up faster without loading the full scala library), thus we can't reuse the mainargs parser here.
lefou commented 1 month ago

As a side note, I envision some native launcher instead of the current solutions (which could be either Scala Native or Graal VM) and fall back to some less performant but singleton handling of these options in case we can't use a native launcher. Hopefully reducing the scattered logic in multiple places.

Scala Native looks more straight forward, as it could share code with the non-native runner, but I don't think our current library used for the client-server communication, junixsocket, will work with that.

It looks like junixsocket is already prepared to be used with Graal native images, so this could be the better approach.

Unfortunately, I don't have experience in both of these.

alastor1729 commented 1 month ago

@lefou, due to my work-constraints && work-deadlines (for the near future), and through email communication with @lihaoyi...