-
Notifications
You must be signed in to change notification settings - Fork 3
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
Updated message for missing dependencies #123
Conversation
This is mostly relevant to packages managed by NVM, which seem to be located in a special place.
07cffb9
to
bf9a4a1
Compare
e88d880
to
e57315c
Compare
ba573b5
to
1db3ef5
Compare
It was my mistake to open this PR without waiting for CI to pass. I think it is truly ready to merge now. |
@@ -62,7 +60,7 @@ jobs: | |||
platform: [ubuntu-latest, macos-latest, windows-latest] | |||
runs-on: ${{ matrix.platform }} | |||
steps: | |||
- uses: lf-lang/vscode-lingua-franca/.github/actions/build@main | |||
- uses: lf-lang/vscode-lingua-franca/.github/actions/build@update-error-message |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we get rid of the @ref
by referencing the workflow locally, like we do in lingua-franca
now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good modulo comment...
@@ -49,7 +44,10 @@ jobs: | |||
platform: [ubuntu-latest, macos-latest, windows-latest] | |||
runs-on: ${{ matrix.platform }} | |||
steps: | |||
- uses: lf-lang/vscode-lingua-franca/.github/actions/build@main | |||
- uses: lf-lang/vscode-lingua-franca/.github/actions/build@update-error-message |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we check out the repo in this workflow instead of build.yml
, I think we could refer to it with a local path and won't need the @
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried that and it did not work conveniently, but I do not remember why. It could just be that I would need to spend a bit of time rearranging some YAML, but I have not found the time to do that yet.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, then let's not waste time on it. It's not a big deal. Let's merge, adjust the refs, and only then delete this branch.
This is mostly relevant to packages managed by NVM, which seem to be located in a special place.
Fixes #119. This does not actually fix the PATH, but it does at least update the error message so that the message suggests the correct solution and is not misleading.
I believe that we can fix the PATH if we really want to by doing some hacking with the embedded terminal. However, I anticipate that such a solution would cause bugs and maintainability problems that I do not want to face right now.