Skip to content

Update .vscode/tasks.json to run Debug Quarkus command with missing mvnw#578

Merged
datho7561 merged 1 commit intoredhat-developer:masterfrom
JessicaJHee:572-wrapper
Feb 6, 2023
Merged

Update .vscode/tasks.json to run Debug Quarkus command with missing mvnw#578
datho7561 merged 1 commit intoredhat-developer:masterfrom
JessicaJHee:572-wrapper

Conversation

@JessicaJHee
Copy link
Copy Markdown
Member

Fixes #572

Signed-off-by: Jessica He jhe@redhat.com

Copy link
Copy Markdown
Member

@rgrunber rgrunber left a comment

Choose a reason for hiding this comment

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

If I take your change and a quarkus project (eg. config-quickstart) with no .vscode/ folder and just call the Debug Quarkus command, it generates everything as before, but the debugging doesn't start.

It looks like the last line of startDebugging :

await debug.startDebugging(workspaceFolder, debugConfig); never runs.

I think part of the issue is that getQuarkusDevDebugConfig is also used by waitUntilTaskExists as the last step up creating the launch config in createLaunchConfig.

I also noticed shouldUpdateTaskCommand() is returning undefined even when the wrapper command is still there. I'd have a look at getWrapperPathFromBuildFile as I think that isn't finding the wrapper.

@JessicaJHee
Copy link
Copy Markdown
Member Author

@rgrunber you were correct! I needed to use another way to find the wrapper command. It should work fine now

Comment thread src/utils/tasksUtils.ts Outdated
Copy link
Copy Markdown
Member

@rgrunber rgrunber left a comment

Choose a reason for hiding this comment

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

Overall, this works in the general case. Just some things to polish up and one thing to fix.

One thing to keep in mind that I don't think we need to deal with here is priority. For example, let's say you can't locate the wrapper in the project folder, so you use the default, which works. What happens if the wrapper is put back/re-introduced. Should we detect it and refresh again ? We can address this an another issue if we really feel we should handle this.

Comment thread src/utils/tasksUtils.ts Outdated
Comment thread src/wizards/debugging/TaskCreator.ts
Copy link
Copy Markdown
Member

@rgrunber rgrunber left a comment

Choose a reason for hiding this comment

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

Nice. This even gives priority to the project wrapper (if it exists) ! Just one small thing to address and I think we can merge.

const launchJson = workspace.getConfiguration('launch', this.workspaceFolder.uri);
const configurations: DebugConfiguration[] = launchJson.get<DebugConfiguration[]>('configurations');
const configurations: DebugConfiguration[] = launchJson.get<DebugConfiguration[]>('configurations')
.filter(task => task['preLaunchTask'] != this.quarkusBuildSupport.getQuarkusDevTaskName(this.workspaceFolder, this.projectFolder));
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Is there any reason to use preLaunchTask as opposed to name ? If someone wants to have a launch that uses our pre-defined quarkus:dev task, that's fine. No reason that multiple can't exist. The only thing is we shouldn't allow duplicate launches, which we can interpret to mean launches with the same name.

@datho7561 datho7561 self-requested a review February 6, 2023 19:26
Signed-off-by: Jessica He <jhe@redhat.com>
Copy link
Copy Markdown
Member

@rgrunber rgrunber left a comment

Choose a reason for hiding this comment

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

Looks mostly fine to me. If @datho7561 is fine with it, feel free to merge.

Copy link
Copy Markdown
Contributor

@datho7561 datho7561 left a comment

Choose a reason for hiding this comment

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

This works well for me. Thanks, Jessica!

@datho7561 datho7561 merged commit a94b75f into redhat-developer:master Feb 6, 2023
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.

when no ./mvnw exist cannot debug quarkus

3 participants