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

Fix mill script for parameter starting with -i #3127

Merged
merged 1 commit into from
Apr 23, 2024

Conversation

romain-gilles-ultra
Copy link
Contributor

@romain-gilles-ultra romain-gilles-ultra commented Apr 23, 2024

Context

There is an issue in the build pipeline the following job is falling:

./mill --import ivy:io.chris-kipp::mill-github-dependency-graph::0.2.5 showNamed io.kipp.mill.github.dependency.graph.Graph/generate
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed

  0     0    0     0    0     0      0      0 --:--:-- --:--:-- --:--:--     0
  0     0    0     0    0     0      0      0 --:--:--  0:00:01 --:--:--     0
  0 5[7](https://github.com/com-lihaoyi/mill/actions/runs/8752465078/job/24020171634#step:5:8).3M    0 19288    0     0   9993      0  1:40:23  0:00:01  1:40:22  9993
 94 57.3M   94 54.1M    0     0  22.1M      0  0:00:02  0:00:02 --:--:-- 22.1M
[10](https://github.com/com-lihaoyi/mill/actions/runs/8752465078/job/24020171634#step:5:11)0 57.3M  100 57.3M    0     0  23.3M      0  0:00:02  0:00:02 --:--:-- 23.3M
Preparing Java 17.0.10 runtime; this may take a minute or two ...
[info] compiling 3 Scala sources to /home/runner/work/mill/mill/out/mill-build/compile.dest/classes ...
[info] done compiling
Parsing exception Position 1:[14](https://github.com/com-lihaoyi/mill/actions/runs/8752465078/job/24020171634#step:5:15), found "=./mill"
Error: Process completed with exit code 1.

The issue is coming from the ./mill sh script here:

 # first arg is a long flag for "--interactive" or starts with "-i"
if [ "$1" = "--bsp" ] || [ "${1%"-i"*}" != "$1" ] || [ "$1" = "--interactive" ] || [ "$1" = "--no-server" ] || [ "$1" = "--repl" ] || [ "$1" = "--help" ] ; then
  # Need to preserve the first position of those listed options
  MILL_FIRST_ARG=$1
  shift
fi

If you pass -i or --import it will match the if statement and more precisely the [ "${1%"-i"*}" != "$1" ] part of the if statement.

Solution

Looking at the POSIX documentation of the: POSIX shell variables and parameter substitution

${parameter%pattern}
${parameter%%pattern}

If the POSIX shell pattern matches the end of the value of parameter, then the value of this substitution is the value
of parameter with the matched portion deleted; otherwise, the value of parameter is substituted. In the first form,
the smallest matching pattern is deleted; in the second form, the largest matching pattern is deleted.

It sounds like % matches and substitutes the end of the parameter which explains the issue.
While the following approach uses the beginning of the parameter:

${parameter#pattern}
${parameter##pattern}

If the POSIX shell pattern matches the beginning of the value of parameter, then the value of this substitution is the
value of parameter with the matched portion deleted; otherwise, the value of parameter is substituted. In the first
form, the smallest matching pattern is deleted; in the second form, the largest matching pattern is deleted.

So by replacing [ "${1%"-i"*}" != "$1" ] by [ "${1#"-i"}" != "$1" ] it should solve it 🤞

@lihaoyi
Copy link
Member

lihaoyi commented Apr 23, 2024

Can you write a proper PR description explaining what you believe the problem is and why this is the correct solution?

@romain-gilles-ultra
Copy link
Contributor Author

Hi @lihaoyi
Sorry for this PR hope it's better now 🤗

@lihaoyi
Copy link
Member

lihaoyi commented Apr 23, 2024

@romain-gilles-ultra thanks! Can you verify that passing in flags like ./mill -i -j 4 __.compile and ./mill -ij4 __.compile continues to work

@romain-gilles-ultra
Copy link
Contributor Author

Hi @lihaoyi
Sorry for the delay but yes it works ;)

Copy link
Member

@lefou lefou left a comment

Choose a reason for hiding this comment

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

Thank you!

@lefou lefou merged commit c0ae632 into com-lihaoyi:main Apr 23, 2024
38 checks passed
@lefou lefou added this to the 0.11.8 milestone Apr 23, 2024
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