Move sketch size calculation to Golang #189

facchinm · 2016-11-11T14:20:22Z

ATM Java IDE takes care of calculating the size of a sketch (https://github.com/arduino/Arduino/blob/ec2e9a642a085b32701cf81297ee7c1570177195/arduino-core/src/processing/app/debug/Sizer.java)
This function can be safely moved to the builder BUT there are a couple of gotchas:

The i18n layer only accepts strings, so the numbers are converted to strings before "logging" them. This result in "thousands" not being separated by commas or dots (based on the locale)
- The error path is "concurrent" (Java side) with the stdout path, so the output is perfect when run in go but becomes confused when displayed through "MessageSiphon". The 200ms delay before returning an error solves this (in a very ugly way)

The commits should be squashed and the constants moved to constants.go

@cmaglie @matteosuppo @00alis @mastrolinux

matteosuppo · 2016-11-15T10:17:51Z

I'm getting

Sketch uses 3624 bytes (12%!)(MISSING) of program storage space. Maximum is 28672 bytes.
Global variables use 148 bytes (5%!)(MISSING) of dynamic memory, leaving 2412 bytes for local variables. Maximum is 2560 bytes.

Is it a locale issue?

facchinm · 2016-11-15T10:38:39Z

@matteosuppo Could be... Which locale are you using? Mine is en_GB.UTF-8

matteosuppo · 2016-11-15T10:52:03Z

admin@api-01-dev:~/builder$ locale
locale: Cannot set LC_CTYPE to default locale: No such file or directory
locale: Cannot set LC_ALL to default locale: No such file or directory
LANG=en_US.UTF-8
LANGUAGE=
LC_CTYPE=en_EN.UTF-8
LC_NUMERIC=it_IT.UTF-8
LC_TIME=it_IT.UTF-8
LC_COLLATE="en_US.UTF-8"
LC_MONETARY=it_IT.UTF-8
LC_MESSAGES="en_US.UTF-8"
LC_PAPER=it_IT.UTF-8
LC_NAME=it_IT.UTF-8
LC_ADDRESS=it_IT.UTF-8
LC_TELEPHONE=it_IT.UTF-8
LC_MEASUREMENT=it_IT.UTF-8
LC_IDENTIFICATION=it_IT.UTF-8
LC_ALL=

ArduinoBot · 2016-11-17T14:21:56Z

✅ Build completed.

⬇️ Build URL: http://downloads.arduino.cc/PR/arduino-builder/arduino-builder-189.zip

ℹ️ To test this build:

Replace arduino-builder binary (you can find it where you installed the IDE) with the provided one

matteosuppo · 2016-11-18T14:59:57Z

Works for me

cmaglie · 2016-11-23T11:54:15Z

src/arduino.cc/builder/phases/sizer.go

+	// force multiline match prepending "(?m)" to the actual regexp
+
+	if len(properties["recipe.size.regex"]) > 0 {
+		textRegexp := regexp.MustCompile("(?m)" + properties["recipe.size.regex"])


regexp.Compile should be used instead of regexp.MustCompile because is not said that recipe.size.regex contains a valid regular expression. In case of error we should print a warning and ignore the regexp as it was missing.

This also means the printing code above should be changed to not print sketch size when the regex is missing.

matthijskooijman · 2016-11-23T15:49:16Z

src/arduino.cc/builder/phases/sizer.go

+
+	if maxTextSizeString == "" {
+		return nil
+	}


Why this limitation? Even for boards without a max size, showing the current size is useful (and the IDE does it currently).

I agree, but the original code returns if no max size is specified https://github.com/arduino/Arduino/blob/21ff728c59c1a2fb138348f4e1f6cb4999ff27f5/arduino-core/src/cc/arduino/Compiler.java#L303 .
In fact, max sketch size is compulsory right now 😄

Ah, missed that part. I probably remembered the data max size being optional, no the text size. Still, it might be nice to make the max text size optional now, while we're here?

It has been compulsory from the beginning (from IDE 1.0 era), making it optional now will not provide any benefit IMHO.

matthijskooijman · 2016-11-23T15:50:59Z

src/arduino.cc/builder/phases/sizer.go

+	maxDataSize := -1
+
+	if maxDataSizeString != "" {
+		maxDataSize, _ = strconv.Atoi(maxDataSizeString)


What does Atoi return on an error? Is the first return value even defined in that case? Or should the err value (that is now ignored) be checked instead? This also applies to some other Atoi calls.

Failing Atoi should, in fact, report an error. These are problems normally encountered by core devs, so a stacktrace could be better than an error message... Anyway, actual java code doesn't perform any check.

What do you mean by "report an error"? Panic?

Given that Atoi returns (int, err), I would expect it will return a non-nil error value, but I'm wondering what the returned int value is in that case.

matthijskooijman · 2016-11-23T15:55:29Z

src/arduino.cc/builder/phases/sizer.go

+	// force multiline match prepending "(?m)" to the actual regexp
+
+	if len(properties["recipe.size.regex"]) > 0 {
+		textRegexp := regexp.MustCompile("(?m)" + properties["recipe.size.regex"])


This also means the printing code above should be changed to not print sketch size when the regex is missing.

matthijskooijman · 2016-11-23T15:56:48Z

src/arduino.cc/builder/phases/sizer.go

+	}
+
+	logger.Fprintln(os.Stdout, logLevel, "Sketch uses {0} bytes ({2}%%) of program storage space. Maximum is {1} bytes.", strconv.Itoa(textSize), strconv.Itoa(maxTextSize), strconv.Itoa(textSize*100/maxTextSize))
+	if dataSize > 0 {


This hides the message when dataSize is 0, though that can still be a valid size. Perhaps the "error" value should be < 0?

dataSize should be initialized as -1, as in the Java code, fixing it! Thanks for spotting 😉

ArduinoBot · 2016-11-23T16:42:20Z

✅ Build completed.

⬇️ Build URL: http://downloads.arduino.cc/PR/arduino-builder/arduino-builder-189.zip

ℹ️ To test this build:

Replace arduino-builder binary (you can find it where you installed the IDE) with the provided one

matthijskooijman · 2016-11-23T16:57:10Z

Looks good. As for "to be squashed" commits, do you know about git commit --fixup (combined with git rebase --interactive --autosquash) that can somewhat automate this?

facchinm · 2016-11-23T17:01:12Z

Sure, I only wanted to keep track of the changes to avoid overwriting your comments 😄
If everything is ok (and approved by @cmaglie ) I'll fixup the commits before committing

matthijskooijman · 2016-11-23T17:16:34Z

My suggestion would be to use --fixup, which creates a new commit with a special commit message that you could have pushed now, and then use the --autosquash later to squash to commit in the right place (without having to think about where that is). Not really relevant here, since I think you'll squash them into a single commit, but it can be useful for bigger PRs.

cmaglie · 2016-11-24T13:19:46Z

I've added some more errors check and a test.

Signed-off-by: Martino Facchin <[email protected]>

This helps creating the test on the next commit. Signed-off-by: Cristian Maglie <[email protected]>

Signed-off-by: Cristian Maglie <[email protected]>

Signed-off-by: Martino Facchin <[email protected]>

ArduinoBot · 2016-11-24T15:51:58Z

✅ Build completed.

⬇️ Build URL: http://downloads.arduino.cc/PR/arduino-builder/arduino-builder-189.zip

ℹ️ To test this build:

Replace arduino-builder binary (you can find it where you installed the IDE) with the provided one

facchinm force-pushed the sizer_move_pr branch from a3e2f29 to 1b08e44 Compare November 17, 2016 11:43

cmaglie requested changes Nov 23, 2016

View reviewed changes

matthijskooijman reviewed Nov 23, 2016

View reviewed changes

cmaglie approved these changes Nov 24, 2016

View reviewed changes

facchinm and others added 5 commits November 24, 2016 16:37

Move size calculation from Java IDE

0802e27

Fix double indirection for %% literal

34ae019

Signed-off-by: Martino Facchin <[email protected]>

Factored out size computation function.

b60ac1c

This helps creating the test on the next commit. Signed-off-by: Cristian Maglie <[email protected]>

Added tests for sizer

65aa1e6

Signed-off-by: Cristian Maglie <[email protected]>

Move sizer after PrintUsedLibraries

3a4c024

Signed-off-by: Martino Facchin <[email protected]>

facchinm force-pushed the sizer_move_pr branch from 0444a12 to 3a4c024 Compare November 24, 2016 15:50

facchinm merged commit 771ccbf into arduino:master Nov 28, 2016

facchinm deleted the sizer_move_pr branch November 29, 2016 10:59

cmaglie added this to the 1.3.22 milestone Dec 19, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move sketch size calculation to Golang #189

Move sketch size calculation to Golang #189

facchinm commented Nov 11, 2016 •

edited

Loading

matteosuppo commented Nov 15, 2016 •

edited

Loading

facchinm commented Nov 15, 2016

matteosuppo commented Nov 15, 2016 •

edited

Loading

ArduinoBot commented Nov 17, 2016

matteosuppo commented Nov 18, 2016

cmaglie Nov 23, 2016

matthijskooijman Nov 23, 2016

matthijskooijman Nov 23, 2016

facchinm Nov 23, 2016

matthijskooijman Nov 23, 2016

cmaglie Nov 24, 2016

matthijskooijman Nov 23, 2016

facchinm Nov 23, 2016

matthijskooijman Nov 23, 2016

matthijskooijman Nov 23, 2016

matthijskooijman Nov 23, 2016

facchinm Nov 23, 2016

ArduinoBot commented Nov 23, 2016

matthijskooijman commented Nov 23, 2016

facchinm commented Nov 23, 2016

matthijskooijman commented Nov 23, 2016

cmaglie commented Nov 24, 2016

ArduinoBot commented Nov 24, 2016

Move sketch size calculation to Golang #189

Move sketch size calculation to Golang #189

Conversation

facchinm commented Nov 11, 2016 • edited Loading

matteosuppo commented Nov 15, 2016 • edited Loading

facchinm commented Nov 15, 2016

matteosuppo commented Nov 15, 2016 • edited Loading

ArduinoBot commented Nov 17, 2016

matteosuppo commented Nov 18, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ArduinoBot commented Nov 23, 2016

matthijskooijman commented Nov 23, 2016

facchinm commented Nov 23, 2016

matthijskooijman commented Nov 23, 2016

cmaglie commented Nov 24, 2016

ArduinoBot commented Nov 24, 2016

facchinm commented Nov 11, 2016 •

edited

Loading

matteosuppo commented Nov 15, 2016 •

edited

Loading

matteosuppo commented Nov 15, 2016 •

edited

Loading