Pre- & Post- Build Hooks

[Wackerbarth]

This provides hooks to allow automation of build version numbers and other actions based on recipes in the platform(.local) files

[matthijskooijman]

Sounds like a great change to me that adds flexibility for people that need it.

Any chance you could also add a preupload and postupload hook? I'm using an external serial monitor, and those would allow me to automatically kill my serial monitor before upload instead of having the upload fail.

Finally, if you put "Closes #2732" and/or "References #2732" in the commit messages, the issue will be autoclosed when merged (or just referenced). You can interactively rebase your branch to reword the commit messages and then force push to replace the commits here, if you want.

[Wackerbarth]

@matthijskooijman -- Adding additional hooks is a good idea.

Unfortunately, the obvious place to add the hook that you suggested does not have the preferences context available.

Perhaps you can help me find the appropriate place to add this.

[ffissore]

Lovely

[ffissore]

@ArduinoBot build this please

[ArduinoBot]

Build successful. Please test this code using one of the following:
http://arduino.cc/download.php?f=/javaide/pull_requests/arduino-PR-2738-BUILD-212-linux32.tar.xz
http://arduino.cc/download.php?f=/javaide/pull_requests/arduino-PR-2738-BUILD-212-linux64.tar.xz
http://arduino.cc/download.php?f=/javaide/pull_requests/arduino-PR-2738-BUILD-212-windows.zip
http://arduino.cc/download.php?f=/javaide/pull_requests/arduino-PR-2738-BUILD-212-macosx.zip
http://arduino.cc/download.php?f=/javaide/pull_requests/arduino-PR-2738-BUILD-212-macosx-java-latest.zip

[Wackerbarth]

The MacOSX versions work for my purpose as I had intended.
However, before it becomes a "legacy" in the IDE, I would appreciate feedback on the naming of the parameters.

The example that I am using is:

recipe.hooks.prebuild.action=/usr/local/bin/generate_version_for {build.source.path} {build.path}/_Version.h

You can also have multiple actions

recipe.hooks.postbuild_2.action= ...
recipe.hooks.postbuild_1.action= ...

Is there a preferable syntax?

[ffissore]

Yes. In boards.txt we used a dot notation for multiple values. See https://github.com/arduino/Arduino/blob/master/hardware/arduino/avr/boards.txt#L10-L13

They should become

recipe.hooks.postbuild.0.action= ...
recipe.hooks.postbuild.1.action= ...

This makes it easier to parse the properties: processing.app.helpers.PreferencesMap#subTree(java.lang.String) called with recipe.hooks.postbuild will generate a new Map with key 0.action and value the recipe. You can then iterate through the EntrySet and call the recipes. Less string parsing and less code

[Wackerbarth]

@ffissore - Thanks for your input. Having copied the code fragment from another part of the code dealing with recipe patterns, the matcher will handle either case. It does a prefix- match on "recipe.hooks.postbuild" and postfix- match on ".action". It executes the patterns in sorted order.

My bigger concern is the advisability of using those particular strings rather than some other. For example, I used "action" rather than "pattern". Also should this be in the "recipe" namespace? etc.
There are subtle implications in those choices and I don't pretend to have a good grasp of the overall scheme.

Also, in order to access the git repository of the source, I added "build.source.path" to the context.
I certainly do not wish to pollute the space with unnecessary/redundant items. Although I didn't find it, there might already be an alternative available.

[PaulStoffregen]

This pull request contains a lot of unnecessary changes related to white space.

While cleaning up stray white space may be nice, merging such a pull request will needlessly break other pending pull requests and patches.

[Wackerbarth]

@PaulStoffregen -- So, is it your position that we should continue to perpetuate unnecessary whitespace and thereby cause, in perpetuity, chronic conflicts which affect everyone subsequently editing the file?

In the review process, differences caused by failure to conform to a precise coding standard simply obscure the substantive changes.

For decades, it has been a "Best Practice" to have automated tools to verify that all submissions meet formatting standards before they are accepted in a permanent repository. Decent modern editors also have functions to assist in formatting to such standards.

[ffissore]

@Wackerbarth I agree action may be confusing, since every other command is specified with a pattern. Please comply with the dot notation and use pattern instead of action

recipe.hooks.postbuild.0.pattern=

I agree with @PaulStoffregen about unnecessary modifications, and it's our fault since we don't provide a formatter configuration. Hence, the rule of thumb is to modify as few lines as possible, leaving the others, for ugly they may look, untouched. However I understand editors such as Eclipse may be hard to stop at formatting code.
Please do whatever you can do for limiting the patch to the actual additions you're proposing (which are cool btw, otherwise there wouldn't be such a high comment rate)
Since it's your branch and your code, a force push is perfectly fine.

[PaulStoffregen]

@Wackerbarth - Yes, I suppose when you put it that way, I am saying we should continue to perpetuate unnecessary whitespace.

But really, what I'm saying is patches that fix stray white space outside the area they actually modify cause other patches to break unnecessarily.

From the tone of your message, it seems you bitterly hate stray whitespace. I don't like it either. But I believe stray whitespace is much less harmful that breaking other pull requests and patches.

[Wackerbarth]

"action" changed to "pattern"
The ".0." notation already worked.

Yes, I really think that you should revisit the issue of canonical formatting, trailing whitespace in particular.
Having to maintain it really is a chronic burden. I went to the effort to separate those changes into a separate commit so that others could cherry-pick it into any un-merged branches.

Since non-fast-forward-able merges create really difficult-to-audit code, I suggest that they be minimized.
Perhaps you could adopt a policy of refusing any merges that extend across tags and require that those pending changes be rebased accordingly.

Then, as the last part of tagging, One system-wide cosmetic cleanup commit can be made.
Resolving that as part of the rebase operation on the "open" branches should not be difficult.

Eventually, you can migrate to a pre-commit hook in git that enforces canonical formatting.

[ffissore]

Thank you @Wackerbarth for your patch and understanding