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

Feat: Add diagnostics for embedded languages #18

Merged

Conversation

idillon-sfl
Copy link
Member

No description provided.

@idillon-sfl idillon-sfl changed the title Feat: Add embedded diagnostic Feat: Add diagnostics for embedded languages Dec 1, 2023
@deribaucourt
Copy link
Member

I get this diagnostic from the shellcheck extension in all .bb files
image
Could you identify a way to filter out diagnostics which do not point to an embedded language section?
Is it a good thing that we get diagnostics outside of bash IDE? I think so.

@deribaucourt
Copy link
Member

The problems view also contains the original diagnostics in the temporary files
image

Is it possible to hide them?

@deribaucourt
Copy link
Member

After modifying and saving the file a bit, the wrong line is reported. I guess it corresponds to the virtual file line
image

@deribaucourt
Copy link
Member

I get this diagnostic from the shellcheck extension in all .bb files image Could you identify a way to filter out diagnostics which do not point to an embedded language section? Is it a good thing that we get diagnostics outside of bash IDE? I think so.

Also you could add the shebang to get rid of the error for users of this extension.

@deribaucourt
Copy link
Member

After modifying and saving the file a bit, the wrong line is reported. I guess it corresponds to the virtual file line image

My generated temporary weirdly ends up with a lot of empty newlines aftern modify/save cycles

@idillon-sfl idillon-sfl force-pushed the add-embedded-diagnostic branch 3 times, most recently from 2fdfbed to ab6a2c8 Compare December 12, 2023 00:09
@idillon-sfl
Copy link
Member Author

The problems view also contains the original diagnostics in the temporary files image

Is it possible to hide them?

The "temporary files" appearing in the "problems" tab is very frustrating. In fact, this is what motivated the use of the vscode API for handling these documents. My first implementation was creating new files on every edit, and the problems would just accumulate for each of these files until vscode would make some sort of garbage collection. This is the best I could do for now.

I've been hoping maybe people will find some unexpected usefulness to see their code in a real Python on Bash file, but I am not very optimistic about it.

Until we find a better solution, the "problems" tab has a filter and those files can be hidden with !workspaceStorage or whatever. Maybe we should mention this limitation in the readme and suggest that workaround?

@idillon-sfl
Copy link
Member Author

After modifying and saving the file a bit, the wrong line is reported. I guess it corresponds to the virtual file line image

After modifying and saving the file a bit, the wrong line is reported. I guess it corresponds to the virtual file line image

My generated temporary weirdly ends up with a lot of empty newlines aftern modify/save cycles

I failed to replicate

@idillon-sfl
Copy link
Member Author

I get this diagnostic from the shellcheck extension in all .bb files image Could you identify a way to filter out diagnostics which do not point to an embedded language section? Is it a good thing that we get diagnostics outside of bash IDE? I think so.

Also you could add the shebang to get rid of the error for users of this extension.

done

@idillon-sfl
Copy link
Member Author

Tests fail since the rebase. I don't get it :/

@deribaucourt
Copy link
Member

Tests fail since the rebase. I don't get it :/

There was a race condition that slipped into staging tests. Rebase again and you should be fine.

@deribaucourt
Copy link
Member

The problems view also contains the original diagnostics in the temporary files image
Is it possible to hide them?

The "temporary files" appearing in the "problems" tab is very frustrating. In fact, this is what motivated the use of the vscode API for handling these documents. My first implementation was creating new files on every edit, and the problems would just accumulate for each of these files until vscode would make some sort of garbage collection. This is the best I could do for now.

I've been hoping maybe people will find some unexpected usefulness to see their code in a real Python on Bash file, but I am not very optimistic about it.

Until we find a better solution, the "problems" tab has a filter and those files can be hidden with !workspaceStorage or whatever. Maybe we should mention this limitation in the readme and suggest that workaround?

If it's possible, update the config (.vscode/settings.json) automatically to filter out those messages.

@deribaucourt
Copy link
Member

After modifying and saving the file a bit, the wrong line is reported. I guess it corresponds to the virtual file line

My generated temporary weirdly ends up with a lot of empty newlines aftern modify/save cycles

I failed to replicate

Still happens on this branch. steps to replicate:

  • open poky/meta/classes/create-spdx-2.2.bbclass
  • some diagnostics are brought up that are very interesting by the way
  • Add an empty new line at add_download_packages() as illustrated
  • Save file
  • Diagnostics are all moved to the top of the file as illustrated
def add_download_packages(d, doc, recipe):

    import os.path
    from bb.fetch2 import decodeurl, CHECKSUM_LIST
    import bb.process

image
Corresponding virtual file diagnostic:
image

Also
image

You should fix what is happening on file modification+save but also decide what to do if you can't map the diagnostic virtual file line to the bitbake recipe embedded language lines.

@idillon-sfl idillon-sfl force-pushed the add-embedded-diagnostic branch from a2dc95f to f52d23e Compare December 12, 2023 17:25
@idillon-sfl
Copy link
Member Author

I still don't manage to replicate the errors. I only noticed sometimes it is slow to update when I save the document and the parsing is running in the background. Maybe this is the issue you have?

What is your version of vscode?

@idillon-sfl
Copy link
Member Author

If it's possible, update the config (.vscode/settings.json) automatically to filter out those messages.

This is something I tried. I failed to find any such setting. It might be impossible.

@deribaucourt
Copy link
Member

I still don't manage to replicate the errors. I only noticed sometimes it is slow to update when I save the document and the parsing is running in the background. Maybe this is the issue you have?

What is your version of vscode?

It doesn't seem to get fixed by waiting.
I'm on vscode 1.84.2
I ran the extension debugger and found that in my case, as soon as I add a new line (before even saving), the python diagnostics get updated but the lines from getOriginalDocRange are incorrect. Even if I revert the new line, the diagnostics line index keep being offset by the same number.

@deribaucourt
Copy link
Member

If it's possible, update the config (.vscode/settings.json) automatically to filter out those messages.

This is something I tried. I failed to find any such setting. It might be impossible.

Maybe virtual documents would help? But OK for now.

@idillon-sfl idillon-sfl force-pushed the add-embedded-diagnostic branch from fb5f3c3 to 2d81a2e Compare December 14, 2023 21:12
@idillon-sfl
Copy link
Member Author

The issue with "trimTrailingWhitespace" should be fixed now

@deribaucourt
Copy link
Member

The issue with "trimTrailingWhitespace" should be fixed now

The diagnostics are fine, but the temporary files get opened each time I modify the .bb
image

@idillon-sfl idillon-sfl force-pushed the add-embedded-diagnostic branch from 2d81a2e to dc3db6b Compare December 15, 2023 19:42
@idillon-sfl
Copy link
Member Author

The issue with "trimTrailingWhitespace" should be fixed now

The diagnostics are fine, but the temporary files get opened each time I modify the .bb image

indeed :/

I reverted completely this solution and tried something else.

A nice solution would have been to turn off the setting only for the "embedded-languages" folder. However, it seems vscode does not offer that sort of granularity for its settings. For this reason, I turned it off instead only for python and shellscript, so at least people who want to activate that setting will still have it while writing bitbake code.

Unfortunately, contrary to the previous solution, I've been unable to make the test work on it.

@deribaucourt
Copy link
Member

Thanks for your detailed explanations of the limitations. This feature is very promising, I hope we can find other workarounds of the current VSCode limitations.
The current branch works nice except that sometimes the temporaries do get opened. This is a race condition in updateEmbeddedLanguageDocFile if save() is not called soon enough. I guess this comes from the await logic. If another event happens like a parseOnsave completing, or another modification to the file gets applied we can end up in this race condition. Except from that, the feature seems to work perfectly. Unforunately there doesn't seem to be any way to close or prevent opening the temporary files. So we'll have to fix that race condition. Do you have any idea how we could fix it?
COuld you make progress with the tests as well?

@idillon-sfl idillon-sfl force-pushed the add-embedded-diagnostic branch from 5c3c764 to 67e71fb Compare January 4, 2024 00:22
@ioioioio
Copy link

ioioioio commented Jan 4, 2024

I found a way to close tabs as they open. They still can be seen appearing and disappearing once in a while in the tab bar, but at least the user does not have to close them manually.

If I type random characters very fast, once in a while a tab will try to close while it still has non-saved changes (tab.isDirty), and a prompt will appear. This might be annoying if you type very very fast, but hopefully there's no one who code that fast?

It is not ready to merge, I still have to clean up a couple of things. I'd just like to know if you think the behavior is good enough for now.

@deribaucourt
Copy link
Member

Closing the tabs is still a bit hacky UX. It's really a shame we don't have a proper VSCode API for diagnostics here. However having the risk of getting the notification for unsaved modifications is definitely too intrusive. That's very likely due to a race condition when two rapid onDidChangeContent happen while a onDidChangeTabs has already started. Maybe moving the onDidChangeContent under the client could solve the race condition? That would depend on how VSCode is implemented. A workaround could also be to have a cooldown period for saveEmbeddedLanguageDocs() to space them out: If a request has been handled last ~1s, queue the next one.

Otherwise, could you make another try with the NodeJS file API and notify vscode to refresh the python diagnostics?

@idillon-sfl idillon-sfl force-pushed the add-embedded-diagnostic branch from 67e71fb to 408322b Compare January 4, 2024 21:59
@idillon-sfl
Copy link
Member Author

There's no way to notify vscode to refresh the diagnostics. Apparently the LSP has such a feature, but it is not yet integrated to VS Code.

I tried updating the file with "fs", then updating with a blank change, then saving with vs code api. It won't update the diagnostics.
I tried updating the file with "fs", then updating with the identical content, then saving with vs code api. It gives an error.
I tried deleting the file with the vscode api, then recreating an identical one still with the vscode API and opening it again with the vs code api. It won't update the diagnostics.

I made a change that edits the files only if they are not being updated. After spending a couple of minutes typing junk as fast as I could, I never saw the prompt to save the files on closing the tabs. Once in a while the undesired tabs will stay open for a couple of seconds, but it always closed itself after I stopped typing. It also makes the completion a bit less responsive, but it is still better than what it was before removing the debouncing.

@deribaucourt
Copy link
Member

I got this error in pkgconf.bb, maybe it's something that should be fixed in the virtual document? I'm using bash-ide and timonwong.shellcheck linter.
image

@deribaucourt
Copy link
Member

I really like the current iteration! On my setup, no more "file not saved" popup as well. The diagnostics are very responsive and the modification tab is not that annoying and gets away very fast usually.
I think this is publishable. Could you add TODOs in the relevant parts of the code and explain how we could do better once VSCode has the necessary diagnostics API?

@deribaucourt
Copy link
Member

Also, if I click on the diagnostics for the virtual files in the problems view, they get immediately closed. Users shouldn't really do this adn rather click on the diagnostics for the .bb originals. Is there something we can do? Masking them is not possible, but keeping the file open through clicking when there is no pending file edits?

@idillon-sfl
Copy link
Member Author

I got this error in pkgconf.bb, maybe it's something that should be fixed in the virtual document? I'm using bash-ide and timonwong.shellcheck linter. image

It is because of the override. The :append would be removed for a Python function, but somehow it is not removed for Bash functions. Probably because it did not seem to matter until now. I'll take care of it.

I really like the current iteration! On my setup, no more "file not saved" popup as well. The diagnostics are very responsive and the modification tab is not that annoying and gets away very fast usually. I think this is publishable. Could you add TODOs in the relevant parts of the code and explain how we could do better once VSCode has the necessary diagnostics API?

I have in mind to explain all of it in the readme so the users have a way to know what's going on.

Also, if I click on the diagnostics for the virtual files in the problems view, they get immediately closed. Users shouldn't really do this adn rather click on the diagnostics for the .bb originals. Is there something we can do? Masking them is not possible, but keeping the file open through clicking when there is no pending file edits?

Weird. I thought I was handling it. This line is supposed (according to my experimentations) to prevent them to close when they have been opened by a user action. For example, if I click on a problem from the "problems" tab, the "embedded language file" will open on "preview" mode. I wonder why it does not work for you :/

@idillon-sfl idillon-sfl force-pushed the add-embedded-diagnostic branch from 408322b to 588c47b Compare January 5, 2024 23:04
@idillon-sfl
Copy link
Member Author

The new version does not show warning on do_install:append and inline_python into bash functions. It also adds a TROUBLESHOOTING.md file

@deribaucourt
Copy link
Member

I get some false positive diagnostics still in some recipes, but they can be addressed in new PRs. This one is plenty good now :)
image
image
We also have true positives here which are very helpful!
I suggest in after this is merged that you go across recipes in poky, meta-openembedded, even meta-freescale and look at errors if they are false positives or not.
The results are very interesting. You should present them in the next meeting with @rossburton. shellcheck and Pylance can find better practices in embedded python/bash that can now be detected easily.

deribaucourt
deribaucourt previously approved these changes Jan 8, 2024
@ioioioio
Copy link

ioioioio commented Jan 8, 2024

yes, I'll search for false positives 👌

@idillon-sfl idillon-sfl merged commit 8149488 into yoctoproject:staging Jan 8, 2024
1 check passed
@idillon-sfl idillon-sfl deleted the add-embedded-diagnostic branch January 8, 2024 14:43
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