Skip to content

gRPC sketch content list should stop at first level #1173

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.

Sign up for GitHub

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

Jump to bottom
Closed
ubidefeo opened this issue Feb 5, 2021 · 5 comments · Fixed by #1182
Closed

gRPC sketch content list should stop at first level #1173

ubidefeo opened this issue Feb 5, 2021 · 5 comments · Fixed by #1182
Assignees
@silvanocerza

Comments

@ubidefeo
Copy link

ubidefeo commented Feb 5, 2021

Bug Report

Issue is related to arduino/arduino-pro-ide#416
I deduced that the call for file listing issued by the IDE over gRPC gets the CLI to recursively go through the tree hence returning every file.
This creates the issue linked above.
For how the Sketch is structured, there should be no recursion, and only files at the main Sketch level should be returned.

Environment

  • CLI version (output of arduino-cli version): 0.15.x
  • OS and platform: All

Additional context

arduino/arduino-pro-ide#416
@ubidefeo ubidefeo mentioned this issue Feb 5, 2021
Closed
@silvanocerza
Copy link
Contributor

I investigated a bit and we probably make changes both in the CLI and the IDE to fix this.

The data returned by the gRPC interface is that returned by LoadSketch:

arduino-cli/commands/instances.go

Lines 731 to 753 in 0425c95

func LoadSketch(ctx context.Context, req *rpc.LoadSketchReq) (*rpc.LoadSketchResp, error) {
sketch, err := builder.SketchLoad(req.SketchPath, "")
if err != nil {
return nil, fmt.Errorf("Error loading sketch %v: %v", req.SketchPath, err)
}
otherSketchFiles := make([]string, len(sketch.OtherSketchFiles))
for i, file := range sketch.OtherSketchFiles {
otherSketchFiles[i] = file.Path
}
additionalFiles := make([]string, len(sketch.AdditionalFiles))
for i, file := range sketch.AdditionalFiles {
additionalFiles[i] = file.Path
}
return &rpc.LoadSketchResp{
MainFile: sketch.MainFile.Path,
LocationPath: sketch.LocationPath,
OtherSketchFiles: otherSketchFiles,
AdditionalFiles: additionalFiles,
}, nil
}

If we try to load a sketch structured like this:

TestLoadSketchFolder
├── doc.txt
├── header.h
├── old.pde
├── other.ino
├── s_file.S
├── src
│   ├── dont_load_me.ino
│   └── helper.h
└── TestLoadSketchFolder.ino

the resulting data in LoadSketchResp would be:

  • MainFile: TestLoadSketchFolder.ino
  • LocationPath: <absolute_path_to_sketch_folder>
  • OtherSketchFiles: [old.pde, other.ino]
  • AdditionalFiles: [header.h, s_file.S, src/helper.h]

The IDE loads MainFile, OtherSketchFiles and AdditionalFiles.

To make it work the way we expect it we must make the CLI append to OtherSketchFiles all the supported files from the sketch root folder and put the rest in AdditionalFiles.

The IDE must not open AdditionalFiles as tabs.

This was introduced with PR #926

Would like some feedback from @kittaakos and @ubidefeo before going forward with this.

@kittaakos
Copy link
Contributor

@silvanocerza, I am not sure if I can advise here but your proposal looks good to me. Adjusting the IDE code is not a problem at all, we can filter the additional files from the sketch. I checked how the Marlin project behaves in the Java IDE:
Screen Shot 2021-02-10 at 09 10 22

If the CLI can return with the same resultset, we are good here. The only question I would have is how to open the other files if opening the other files is a requirement at all.

@silvanocerza
Copy link
Contributor

We'll go with the Java IDE behaviour for now. We'll see in the future what to do I guess.

@silvanocerza
Copy link
Contributor

I tried to implent what I suggested above but sadly it can't be done. The CLI uses those properties in the compilation process, changing them would break lots of parts of the legacy package.

Think I'll just a RootFolderFiles that will contain all the files in the sketch root. Nonetheless we'll need to think of a better way of handling files in the IDE.

@kittaakos
Copy link
Contributor

Think I'll just a RootFolderFiles that will contain all the files in the sketch root.

This can be done on the IDE side too. No need for a change now, the IDE can wait until the CLI team figures out what is the correctt way to handle it.

@silvanocerza silvanocerza mentioned this issue Feb 11, 2021
Merged
5 tasks
@rsora rsora reopened this May 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@silvanocerza silvanocerza

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Add RootFolderFiles to gRPC LoadSketchResp arduino/arduino-cli
4 participants
@kittaakos @ubidefeo @silvanocerza @rsora