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

silvanocerza · 2021-07-09T08:44:50Z

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.

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

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 · 2021-07-09T08:47:57Z

commands/instances.go

@@ -842,29 +842,30 @@ func Upgrade(ctx context.Context, req *rpc.UpgradeRequest, downloadCB DownloadPr

 // 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


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.

cmaglie · 2021-07-12T08:40:34Z

arduino/builder/sketch.go

-// 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
-}


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

arduino/builder/sketch_test.go

arduino/sketch/sketch.go

legacy/builder/test/ctags_runner_test.go

per1234 · 2021-07-12T11:49:11Z

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.

silvanocerza · 2021-07-12T13:04:52Z

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.

per1234 · 2021-07-12T16:00:38Z

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.

silvanocerza · 2021-07-12T16:08:33Z

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.

docs/UPGRADING.md

arduino/sketch/sketch_test.go

commands/instances.go

arduino/sketch/sketch_test.go

per1234

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

per1234 · 2021-07-13T10:02:26Z

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.

silvanocerza · 2021-07-13T10:05:18Z

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.

cmaglie

👍🏼

matthijskooijman · 2021-09-08T17:13:08Z

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:

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.
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.
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).
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?

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).

silvanocerza added type: enhancement Proposed improvement topic: code Related to content of the project itself labels Jul 9, 2021

silvanocerza requested a review from a team July 9, 2021 08:44

silvanocerza self-assigned this Jul 9, 2021

silvanocerza commented Jul 9, 2021

View reviewed changes

cmaglie reviewed Jul 12, 2021

View reviewed changes

silvanocerza added 5 commits July 12, 2021 12:59

Refactor codebase to use a single Sketch struct

d6d1653

Add back removed legacy test

2f2e2a6

Changed Sketch struct to not use pointers

db9c761

Fix small comment

8de0dec

Fix checkSketchCasing to work with .pde files too

7d00b83

silvanocerza force-pushed the scerza/sketch-refactor branch from 8bc67da to 7d00b83 Compare July 12, 2021 11:00

silvanocerza force-pushed the scerza/sketch-refactor branch from f852cf7 to 69ef64b Compare July 12, 2021 15:30

Add tests loading Sketch with symlinks

ac517be

silvanocerza force-pushed the scerza/sketch-refactor branch from 69ef64b to ac517be Compare July 12, 2021 16:01

per1234 reviewed Jul 12, 2021

View reviewed changes

docs/UPGRADING.md Outdated Show resolved Hide resolved

per1234 requested changes Jul 13, 2021

View reviewed changes

silvanocerza added 3 commits July 13, 2021 09:53

Update UPGRADING.md

07f3ae5

Clarify a code comment

9cac5aa

Add some more tests for Sketches with symlinks

bf30333

silvanocerza force-pushed the scerza/sketch-refactor branch from ee6b995 to bf30333 Compare July 13, 2021 08:04

per1234 requested changes Jul 13, 2021

View reviewed changes

arduino/sketch/sketch_test.go Outdated Show resolved Hide resolved

Fix Sketch symlinks tests for Windows

9618e9d

per1234 approved these changes Jul 13, 2021

View reviewed changes

silvanocerza changed the title ~~Refactor codebase to use a single Sketch struct~~ [breaking] Refactor codebase to use a single Sketch struct Jul 13, 2021

cmaglie approved these changes Jul 13, 2021

View reviewed changes

silvanocerza merged commit e7d4eaa into master Jul 13, 2021

silvanocerza deleted the scerza/sketch-refactor branch July 13, 2021 10:15

This was referenced Sep 8, 2021

Improve broken symlink and other error handling arduino/go-paths-helper#13

Open

Add support for json, adoc and md sketch files extensions #1235

Merged

matthijskooijman mentioned this pull request Sep 8, 2021

Improve handling of read errors during sketch enumeration #1438

Closed

5 tasks

per1234 mentioned this pull request Dec 16, 2021

Hang on symlink loop in library #1593

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

silvanocerza commented Jul 9, 2021 •

edited

silvanocerza Jul 9, 2021

cmaglie Jul 12, 2021

per1234 commented Jul 12, 2021

silvanocerza commented Jul 12, 2021

per1234 commented Jul 12, 2021

silvanocerza commented Jul 12, 2021

per1234 left a comment

per1234 commented Jul 13, 2021

silvanocerza commented Jul 13, 2021

cmaglie left a comment

matthijskooijman commented Sep 8, 2021

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

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

Conversation

silvanocerza commented Jul 9, 2021 • edited

silvanocerza Jul 9, 2021

Choose a reason for hiding this comment

cmaglie Jul 12, 2021

Choose a reason for hiding this comment

per1234 commented Jul 12, 2021

silvanocerza commented Jul 12, 2021

per1234 commented Jul 12, 2021

silvanocerza commented Jul 12, 2021

per1234 left a comment

Choose a reason for hiding this comment

per1234 commented Jul 13, 2021

silvanocerza commented Jul 13, 2021

cmaglie left a comment

Choose a reason for hiding this comment

matthijskooijman commented Sep 8, 2021

silvanocerza commented Jul 9, 2021 •

edited