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

Gradlew not longer used to run dev version of lf cli tools #1988

Merged

Conversation

axmmisaka
Copy link
Collaborator

@axmmisaka axmmisaka commented Sep 1, 2023

This PR fixes #1909.

Right now, we have two ways of executing lfc (mutatis mutandis for lfd and lff):

  1. Use the script built by gradle located in build/install/lf-cli/bin/
  2. Use gradlew to invoke cli:lfc:run, which is handled by ./bin/lfc

The issue is these two methods handle strings differently - specifically, gradlew will parse strings again. If we do not distinguish them and use them interchangeably, we will get screwed (#1909 )

This PR renames the executables in ./bin by adding a -dev suffix to them, to distinguish them from any installed artifacts that may already be on the PATH. In addition, the scripts have been simplified by not letting Gradle handle the invocation of lfc, lfd, etc. because this leads to problems like the one reported in lf-lang/playground-lingua-franca#81 because Gradle swallows exit codes.

util/scripts/launch.ps1 Outdated Show resolved Hide resolved
@lhstrh
Copy link
Member

lhstrh commented Sep 4, 2023

1. Whether to use `./gradlew cli:lfc:run`

2. Should we have `./bin` and `./utils` at all

We've talked about this at quite some length, and the consensus was that it was OK to have these script, but they shouldn't have the same name as the executables built for production (which Gradle places in the build directory).

We resolved to just add -dev to the names of these helper scripts so that there wouldn't be any confusion between installed artifacts and ad-hoc built ones. This question about file names and placement, however, is orthogonal to the change suggested in this PR.

@petervdonovan petervdonovan removed their request for review September 6, 2023 03:00
@lhstrh
Copy link
Member

lhstrh commented Sep 6, 2023

Related: lf-lang/playground-lingua-franca#81

@lhstrh lhstrh changed the title Do not use gradlew to run dev version of lf cli tools Gradlew not longer used to run dev version of lf cli tools Sep 7, 2023
Copy link
Member

@lhstrh lhstrh left a comment

Choose a reason for hiding this comment

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

Let's go ahead and merge this so that other problems dependent on this can be worked on.

@lhstrh lhstrh added the refactoring Code quality enhancement label Sep 7, 2023
@lhstrh lhstrh added this pull request to the merge queue Sep 7, 2023
@axmmisaka
Copy link
Collaborator Author

Let's squash merge this commit because it's relatively small and most commits are small cosmetic surgery.

@lhstrh
Copy link
Member

lhstrh commented Sep 7, 2023

It's already on the merge queue...

Merged via the queue into master with commit 09e243b Sep 7, 2023
41 checks passed
@lhstrh lhstrh deleted the do-not-use-gradlew-to-run-dev-version-of-lf-cli-tools branch September 7, 2023 08:09
else
"${gradlew}" -p "${base}" "cli:${tool}:run" --args="$*"
fi
"${gradlew}" --quiet assemble ":cli:${tool}:assemble"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Using --quiet is a very good idea! However, by dropping the -p it is now not possible anymore to run lfc-dev from an arbitrary directory. For me, this was the main reason for using the script instead of the corresponding gradle run command. I will provide a fix in another PR.

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

Successfully merging this pull request may close these issues.

--json argument is not properly handled by lfc
3 participants