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

Add an app category in Cozy #6

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

bastien-buil
Copy link

@bastien-buil bastien-buil commented Dec 6, 2024

This pull request adds an app category in cozy. The app category is for any files that are inside the directory of the RIOT application. This allows to have a color for all the code that is in the application folder.
Another addition is that application that are not in a subdirectory of the RIOT folder are now correctly regrouped (the files were put in the unspecified directory).

Without the modification:
image

With the modification:
image

More on the modification:

  • r"((?P<path>.+)/)?" → This make the path optional. Without the pull request, files that are in the appdir(ectory) are not detected as being in the appdir, because the line of code required the files to be in a subfolder of the appdir.

The app category is for any files that are inside the directory of the current RIOT application
@chrysn
Copy link
Member

chrysn commented Dec 6, 2024

I've tried this out with in-tree examples (gcoap in my case); the result is that an app category is shown, but it is empty, and the example code is still black. Same for tests. Should this be extended to recognize those, or do you rather see this as an "(out-of-tree) app" category, which should then be shown only for that case?

(I have neither a strong preference nor a good insight, as I myself have only ever patched piecemeal in cosy, so while I currently happen to be the person who does most of the merge button pushing, please treat those like comments from a drive-by reviewer.)

@bastien-buil
Copy link
Author

bastien-buil commented Dec 6, 2024

Thank you for your response.

the result is that an app category is shown, but it is empty, and the example code is still black

It is not an intended behavior if you used the --appdir argument of cosy (As it is given when using RIOT). If you give the --appdir argument, and it is still not working, please give me more information on your configuration (board used, a photo of your result...) so I could try to fix my proposition.

The --appdir argument is the path to the app directory and if not specified, it defaults to ../RIOT/examples/hello-world. Making my proposition useless in this case (as cosy do not know which directory is the directory of the app). If it is problematic, I can try to fix this case (for example, by including the examples RIOT subdirectory in the app category).

On my side, on RIOT with the arduino-nano-33-iot (the result is similar on other boards), gcoap example look like that:
image
The app category contains the information on the files of the example and the examples directory is not displayed.

Should this be extended to recognize those, or do you rather see this as an "(out-of-tree) app" category, which should then be shown only for that case?

I see this as a main category in the tree like core, cpu, boards, ...

chrysn added a commit that referenced this pull request Dec 10, 2024
@chrysn
Copy link
Member

chrysn commented Dec 10, 2024

I did invoke it through RIOT, and had that situation. Around the if d['dir'] == appdir: check I placed a debug statement, and that gave me all lines like COMPARING d['dir']='RIOT' WITH appdir='home/chrysn/git/RIOT/examples/hello-world'.

Note tht my appdir is apparently a full path without leading slash, but I didn't alter any RIOT settings, so that seems to be coming from the build system.

Could we rule out that this is an artifact of being combined with the other changes (can't well test it without those right now)? Could you rebase your changes onto current master?

(And maybe use the opportunity to remove the trailing empty space from the d['path'][0] = "app" line).

@chrysn
Copy link
Member

chrysn commented Dec 10, 2024

Can't keep digging right now, but maybe you find things from a snapshot of a breakpoint in the for line in dump.splitlines() loop:

(Pdb) elffile, prefix, appdir, riot_base
('/home/chrysn/git/RIOT/examples/hello-world/bin/native/hello-world.elf', '', 'home/chrysn/git/RIOT/examples/hello-world', 'riotbuild/riotproject/build|riotbuild/riotproject|home/chrysn/git/RIOT/build|home/chrysn/git/RIOT|RIOT/build|RIOT|riotbuild/riotbase/build|riotbuild/riotbase')
(Pdb) m.groups()
('08049606', 'T', 'main', '/home/chrysn/git/', 'RIOT', 'examples/hello-world/', 'examples/hello-world', 'main.c', 'c', '24')
(Pdb) m.groupdict()
{'addr': '08049606', 'type': 'T', 'sym': 'main', 'dir': 'RIOT', 'path': 'examples/hello-world', 'file': 'main.c', 'line': '24'}
(Pdb) print(d)
{'arcv': '', 'obj': '', 'size': -1, 'alias': [], 'addr': '08049606', 'type': 'T', 'sym': 'main', 'dir': 'RIOT', 'path': ['examples', 'hello-world'], 'file': 'main.c', 'line': '24'}

@bastien-buil
Copy link
Author

bastien-buil commented Dec 11, 2024

Thank you for your responses, it allowed me to find the error I missed.
The issue come from: cad085a that I didn't merge before testing (sorry for that).

In short, as the string "RIOT" is appended in the rbase, RIOT is matched over the appdir for matching the dir.
This is maybe linked to the fact that in (.*/), the .* is in greedy mode (and so, try to match as many characters as possible).

We cannot transform (.*/) in (.*/?) (transform in lazy mode), this would match the first appearance of RIOT in a path containing two times RIOT (like /home/bastien/RIOT/git/RIOT/examples/hello-world/main.c).

I will try to propose a solution before the end of the week (probably by matching the directory "examples", inside the app category) and I will also remove the trailing empty space.

@bastien-buil
Copy link
Author

bastien-buil commented Dec 13, 2024

I have fixed the issue by checking if examples is in path and transforming the path to be in the app category.

The other solution I had (and have looked to implement) was to match the color in the graph of the app category with the example folder, but I found it confusing to have the examples matched to app category and not have the app in the list.

In the picture, the examples folder is replaced by "app" so that files are in the app category:
image

I think the pull request is ready to be reviewed.

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.

2 participants