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

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

Closed
Gedochao opened this issue Nov 10, 2023 · 14 comments · Fixed by #3346
Closed
Labels
bounty bug The issue represents an bug
Milestone

Comments

@Gedochao
Copy link

Gedochao commented Nov 10, 2023


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
Copy link
Member

lefou commented Nov 10, 2023

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
Copy link
Author

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
Copy link
Member

lefou commented Nov 10, 2023

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

@lefou
Copy link
Member

lefou commented Nov 10, 2023

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

if (Arrays.asList("-i", "--interactive", "--no-server", "--repl", "--bsp", "--help").contains(firstArg)) {

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

@lefou
Copy link
Member

lefou commented Nov 10, 2023

Hm, we have it here:

streams.err.println(
"-i/--interactive/--no-server/--bsp must be passed in as the first argument"
)

@lefou lefou added the bug The issue represents an bug label Nov 10, 2023
@Gedochao
Copy link
Author

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

@lefou
Copy link
Member

lefou commented Nov 10, 2023

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
Copy link
Member

lefou commented Nov 10, 2023

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
Copy link

Hi @Gedochao / @lefou,

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

Sincerely,
R.W.

@lihaoyi
Copy link
Member

lihaoyi commented Jul 8, 2024

@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
Copy link

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

  • Please feel free to modify the start-time of this Google Meeting (to better fit your schedule) 🙂

@lihaoyi lihaoyi changed the title -i and --no-server gets ignored with no error when passed as the second option -i and --no-server gets ignored with no error when passed as the second option (200USD Bounty) Jul 22, 2024
@lefou
Copy link
Member

lefou commented Jul 24, 2024

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
Copy link
Member

lefou commented Jul 24, 2024

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
Copy link

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

  • I no longer have time to work on this OSS Mill Issue 🙁
  • However, @wanggang1 ( [email protected] ) will be taking this issue from me / expressed interest in this Mill Issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bounty bug The issue represents an bug
Projects
None yet
4 participants