Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Sign up
Appearance settings

[breaking] Refactor codebase to use a single Sketch struct #1353

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

Merged
silvanocerza merged 10 commits into master from scerza/sketch-refactor
Jul 13, 2021

Conversation

Copy link
Contributor

@silvanocerza silvanocerza commented Jul 9, 2021 •
edited
Loading

Please check if the PR fulfills these requirements

  • The PR has no duplicates (please search among the Pull Requests
    before creating one)
  • The PR follows
    our contributing guidelines
  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)
  • UPGRADING.md has been updated with a migration guide (for breaking changes)
  • What kind of change does this PR introduce?

This enhances the codebase by removing duplicated ways of storing a Sketch data.

  • What is the current behavior?

There are currently 3 different packages that declare a Sketch struct, often keeping track of different informations about a Sketch or using different types for the same underlying data. This made it necessary to have tons of back and forth conversions about the different types often losing information or making it hard to understand the code when working on it.

The 3 different packages are:

  • sketch in arduino/sketch/sketch.go

  • sketches in arduino/sketches/sketches.go

  • types in legacy/builder/types/types.go

  • What is the new behavior?

Now there is only one Sketch struct defined in the sketch of arduino/sketch/sketch.go, it contains all the informations needed
by the codebase.

Yes, only in the exposed public code interface, gRPC and CLI are untouched.

  • Other information:

This fixes #1178.

It's important that we test this in a thorough manner since it touches the legacy package quite a bit.

The handling of symlinks when found in the Sketch folder is also enhanced, previously they caused several issues like #358 and #424, both handled by #421 and #462.
The integration test test_compile_with_sketch_with_symlink_selfloop introduced by #462 used to verify compilation would fail when trying to compile a Sketch with a symlink loop but now that is handled gracefully when a Sketch is loaded internally, so compilation now succeds.
Related to this there is also the unit test TestNewSketchFolderSymlink to verify that loading of a Sketch with symlinks works fine, previously this was TestLoadSketchFolderSymlink in arduino/builder/sketch_test.go but has been moved to arduino/sketch/sketch_test.go.

See how to contribute

@silvanocerza silvanocerza added type: enhancement Proposed improvement topic: code Related to content of the project itself labels Jul 9, 2021
@silvanocerza silvanocerza requested a review from a team July 9, 2021 08:44
// LoadSketch collects and returns all files composing a sketch
func LoadSketch(ctx context.Context, req *rpc.LoadSketchRequest) (*rpc.LoadSketchResponse, error) {
sketch, err := builder.SketchLoad(req.SketchPath, "")
// TODO: This a ToRpc function for the Sketch struct
Copy link
Contributor Author

@silvanocerza silvanocerza Jul 9, 2021

Choose a reason for hiding this comment

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

I didn't want to introduce breaking changes with this refactoring but I think changing the gRPC interface LoadSketchResponse message to reflect the internal Sketch struct would be a nice improvement for the future.

Also I think we should start adding ToRpc/FromRpc functions to structs that need to be converted when necessary, as of now that are some functions that handle those conversions scattered around different packages.

Comment on lines -67 to -98
// simpleLocalWalk locally replaces filepath.Walk and/but goes through symlinks
func simpleLocalWalk(root string, maxDepth int, walkFn func(path string, info os.FileInfo, err error) error) error {

info, err := os.Stat(root)

if err != nil {
return walkFn(root, nil, err)
}

err = walkFn(root, info, err)
if err == filepath.SkipDir {
return nil
}

if info.IsDir() {
if maxDepth <= 0 {
return walkFn(root, info, errors.New("Filesystem bottom is too deep (directory recursion or filesystem really deep): "+root))
}
maxDepth--
files, err := ioutil.ReadDir(root)
if err == nil {
for _, file := range files {
err = simpleLocalWalk(root+string(os.PathSeparator)+file.Name(), maxDepth, walkFn)
if err == filepath.SkipDir {
return nil
}
}
}
}

return nil
}
Copy link
Member

Choose a reason for hiding this comment

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

This function loops through symlinks... we should implement something similar in go-paths-helper to avoid regressions.

per1234 reacted with thumbs up emoji
Copy link
Contributor

per1234 commented Jul 12, 2021

Does this PR introduce a breaking change, and is
titled accordingly?
Nope, hopefully.

Doesn't this PR make significant breaking changes to the API of the Go packages? Are those considered part of Arduino CLI's public API?

It breaks Arduino Lint. I'm OK with breaking changes if they are necessary. But I think it's important to communicate about it to the users.

Copy link
Contributor Author

Good point @per1234, I didn't take that into consideration at all.

Nonetheless am not sure how we could handle breaking changes of the public API, we don't have versioning for it and neither a clearly defined interface for those that want to use it as a library.

I always thought that the breaking changes policy applied only to the CLI and gRPC consumers, changes in the public API is certainly something that we must take into account for the future if we want to keep using the CLI as a library but we first need to define a public/private boundary.

Copy link
Contributor

per1234 commented Jul 12, 2021

am not sure how we could handle breaking changes of the public API

It could be handled the same way as any other breaking change:
https://arduino.github.io/arduino-cli/dev/CONTRIBUTING/#pull-request-checklist

neither a clearly defined interface for those that want to use it as a library.

I've always thought that all exported components of non-internal packages were the interface. This is why I have taken the measure of making the packages of Arduino Lint and arduino/libraries-repository-engine internal, since those projects are not intended to be used as libraries at this time.

I always thought that the breaking changes policy applied only to the CLI and gRPC consumers

The Go library is advertised as one of the "three pillars of Arduino CLI":
https://arduino.github.io/arduino-cli/dev/integration-options/#the-third-pillar-embedding

Embedding the Arduino CLI is limited to Golang applications and requires a deep knowledge of its internals. For the average use case, the gRPC interface might be a better alternative. Nevertheless, this remains a valid option that we use and provide support for.

This is where I got the idea that it was intended to be part of the public API of the project. However, I notice now that almost every interface used by the example program in that "Integration Options" article has been broken in the year since it was written.

Copy link
Contributor Author

It could be handled the same way as any other breaking change:
https://arduino.github.io/arduino-cli/dev/CONTRIBUTING/#pull-request-checklist

Am doing this right now.

I've always thought that all exported components of non-internal packages were the interface. This is why I have taken the measure of making the packages of Arduino Lint and arduino/libraries-repository-engine internal, since those projects are not intended to be used as libraries at this time.

Rightly so I'd say, fact is that the CLI is kinda burdened with old decisions that make it hard to separate the public from the private APIs, it would be cool to do it. With the Arduino Lint it has been easier since we started the project from the ground up with a clear idea in mind of what we wanted it to be, not so much for the CLI I'd say.

The Go library is advertised as one of the "three pillars of Arduino CLI":
https://arduino.github.io/arduino-cli/dev/integration-options/#the-third-pillar-embedding

Yeah, I know that, my main pain point about this is the one I stated above though.

This is where I got the idea that it was intended to be part of the public API of the project. However, I notice now that almost every interface used by the example program in that "Integration Options" article has been broken in the year since it was written.

That part of the documentation needs some love probably, the example are really outdated. I think we must avoid using images for those kind of examples, it makes it harder to update.

Copy link
Contributor

@per1234 per1234 left a comment

Choose a reason for hiding this comment

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

I think it's a really nice improvement Silvano. Thanks for putting up with my nitpickery.

silvanocerza reacted with heart emoji
Copy link
Contributor

per1234 commented Jul 13, 2021

One last suggestion following on the discussion of breaking changes: there are two things which are supposed to be done to communicate breaking changes:
https://arduino.github.io/arduino-cli/latest/CONTRIBUTING/#pull-request-checklist

The UPGRADING.md part is done now, but not yet the use of the "[breaking]" prefix on the PR title. I don't think it's so important to add it to the commit messages in this case because when a PR contains multiple commits as this one does, the squashed commit is titled with the PR title. The primary reason for the instructions to use the prefix on commit messages is because if a PR contains only a single commit then the PR title is not used by the merged commit.

Copy link
Contributor Author

I think it's a really nice improvement Silvano. Thanks for putting up with my nitpickery.

Nitpickery is always welcome. 😁

The UPGRADING.md part is done now, but not yet the use of the "[breaking]" prefix on the PR title. I don't think it's so important to add it to the commit messages in this case because when a PR contains multiple commits as this one does, the squashed commit is titled with the PR title. The primary reason for the instructions to use the prefix on commit messages is because if a PR contains only a single commit then the PR title is not used by the merged commit.

I'll update the PR title, it uses that when squashing and merging.

@silvanocerza silvanocerza changed the title (削除) Refactor codebase to use a single Sketch struct (削除ここまで) (追記) [breaking] Refactor codebase to use a single Sketch struct (追記ここまで) Jul 13, 2021
Copy link
Member

@cmaglie cmaglie left a comment

Choose a reason for hiding this comment

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

👍🏼

@silvanocerza silvanocerza merged commit e7d4eaa into master Jul 13, 2021
@silvanocerza silvanocerza deleted the scerza/sketch-refactor branch July 13, 2021 10:15
Copy link
Collaborator

While revisiting #1201, I came upon this PR and looked at it in a bit more detail. It seems this makes three changes that I'm not entirely sure are intentional:

  1. In sketch.New, open failures are now ignored and the offending file is silently skipped. In general, I would argue that ignoring errors is a bad idea, and if this causes those files to be silently omitted from further processing (i.e. omitted from compilation), that could lead to very puzzling (linker) errors that would be hard to diagnose. I would suggest to not ignore these errors, and instead fail to load the sketch (at least for files that will actually be processed, i.e. files after file extension filtering). Note that some of these errors (that cause a Stat() failure, such as a broken symlink) are currently reported by go-paths-helper already, but I think that is the wrong place (better have arduino-cli have control over this error reporting, so it can apply the reporting after the file extension filtering). I am preparing a PR that implements this.
  2. When collecting the list of files in a sketch, the CVS and RCS were previously omitted, but now only hidden files are omitted. Even though CVS and RCS are old and might not be worth supporting, it looks like this change might have been overlooked. Note that these directories (including some others) are still excluded in the actual build process, but AFAICS not excluding them here means unecessarily enumerating them, uneccessarily keeping them in memory, and probably also unecessarily copying them into the build directory.
  3. Previously, hidden directories (as well as CVS/RCS) where ignored during the tree walk. Now, they are fully enumerated, and only filtered out later. This means that the tree enumeration takes needlessly long, especially when a big hidden directory is present (such as a .git directory).
  4. Previously, hidden files and directories were filtered out anywhere in the tree. Now, it seems that only top-level hidden files or directories are filtered out (I suspect because the filtering happens after enumeration and only seems to check for a . prefix against the enumerated path relative to the sketch directory). But this would also be fixed if the previous point is fixed.

As I said, for the file error handling of point 1, I'm preparing a draft PR. For 2-4, it would probably be good to modify go-paths-helper's ReadDirRecursive to accept a callback that tests file/dir names (and probably an isDir boolean) for inclusion in the result?
per1234 reacted with thumbs up emoji

matthijskooijman added a commit to matthijskooijman/arduino-cli that referenced this pull request Sep 8, 2021 
Before commit e7d4eaa ([breaking] Refactor codebase to use a single
Sketch struct (arduino#1353)), any sketch file that could not be opened would
report a fatal error, but the behavior was changed to silently skip such
failing files instead.
This restores the original behavior and fails to load a sketch of some
of the contained files (after filtering on file extension, so this only
includes files that are actually intended to be processed), instead of
silently excluding the files (which likely produces hard to explain
compilation errors later).
matthijskooijman added a commit to matthijskooijman/arduino-cli that referenced this pull request Jan 12, 2024 
Before commit e7d4eaa ([breaking] Refactor codebase to use a single
Sketch struct (arduino#1353)), any sketch file that could not be opened would
report a fatal error, but the behavior was changed to silently skip such
failing files instead.
This restores the original behavior and fails to load a sketch of some
of the contained files (after filtering on file extension, so this only
includes files that are actually intended to be processed), instead of
silently excluding the files (which likely produces hard to explain
compilation errors later).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cmaglie cmaglie cmaglie approved these changes

@per1234 per1234 per1234 approved these changes

Labels
topic: code Related to content of the project itself type: enhancement Proposed improvement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Merge sketches and sketch packages

AltStyle によって変換されたページ (->オリジナル) /