Skip to content
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

Binaries export must now be explicitly specified #1042

Merged
merged 4 commits into from
Nov 2, 2020

Conversation

silvanocerza
Copy link
Contributor

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)
  • What kind of change does this PR introduce?

A change to an existing feature.

  • What is the current behavior?

Currently, if the --build-path is not specified, compilation is done using a temporary folder generated deterministically given the absolute path of the Sketch selected; the generated binaries are then copied from that folder to <sketch_folder>/build/<fqbn>/.
Using the --dry-run flag the user can stop the CLI from copying the binaries from the build folder to the sketch one.

To upload a sketch compiled using the --dry-run flag the user must specificy the build folder in the temp directory with the --input-dir flag or run again compilation omitting --dry-run, otherwise it will fail.

  • What is the new behavior?

The --dry-run flag is removed.
compile never copies generated binaries to the sketch folder.
The --export-binaries (-e) flag is introduced to copy the binaries from the build folder to the sketch one.
Specifying the --output-dir doesn't require using setting also the --export-binaries flag.
A related settings and env var has been added to avoid the need to always specify the --export-binaries flag: sketch.always_export_binaries and ARDUINO_SKETCH_ALWAYS_EXPORT_BINARIES.

If --input-dir or --input-file is not set when calling upload the command will search for the deterministically created build directory in the temp folder and use the binaries found there.

The gRPC interface has been updated accordingly, dryRun is removed.
I also removed exportFile since it was unused.

  • Does this PR introduce a breaking change?

Yes, this introduces some breaking changes.

  • Other information:

This will also help avoid commiting binaries to a Sketch repository by mistake since no binaries will be in that folder.


See how to contribute

@matthijskooijman
Copy link
Collaborator

Sounds like a good change to me. Dry-run was a bit weird, since it did actually do stuff (copy files to build dir, run compilers, etc.).

bool dryRun = 17; // When set to `true` the compiled binary will not be copied to the export directory.
string export_dir = 18; // Optional: save the build artifacts in this directory, the directory must exist.
bool clean = 19; // Optional: cleanup the build folder and do not use any previously cached build
int32 jobs = 13; // The max number of concurrent compiler instances to run (as `make -jx`). If jobs is set to 0, it will use the number of available CPUs as the maximum.
Copy link
Member

Choose a reason for hiding this comment

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

A note here: to handle correct deprecation policies and binary compatibility the ID of exported grpc functions should never change, only increment and/ore remove.
So if the plan is to remove string exportFile = 13 simply delete that line

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good to know, I had some doubt about it in fact. I'll fix it.

bool dryRun = 17; // When set to `true` the compiled binary will not be copied to the export directory.
string export_dir = 18; // Optional: save the build artifacts in this directory, the directory must exist.
bool clean = 19; // Optional: cleanup the build folder and do not use any previously cached build
string export_dir = 17; // Optional: save the build artifacts in this directory, the directory must exist.
Copy link
Member

@facchinm facchinm Oct 23, 2020

Choose a reason for hiding this comment

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

Still not ok :) export_dir should be 18, and clean 19, while export_binaries becomes 20

Copy link
Contributor Author

Choose a reason for hiding this comment

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

🤦 I'll get to it.

@ubidefeo
Copy link

Works as expected, but I am afraid because of the default behaviour some users' workflows will break.
If a user relies on the currently automatically created build folder to do whatever, then as this gets merged and ends up in nightly we'll have to face some backlash

I do like the behaviour, though.
One silly question: what are the chances that this fails?

the command will search for the deterministically created build directory in the temp folder and use the binaries found there.

@facchinm
Copy link
Member

@ubidefeo

One silly question: what are the chances that this fails?

Same chance of md5 clash on two strings (I agree that md5 is not ideal but every other hash would make the folder name too long with the usual Windows problems we are aware of)

md5SumBytes := md5.Sum([]byte(path))

@silvanocerza
Copy link
Contributor Author

Chance of an md5 clash are pretty slim I think, how many sketches you'd have to compile for that chance?

@ubidefeo
Copy link

@facchinm @silvanocerza
my concern wasn't name clashing, but how repeated compile/upload (and the new --clean flag) might play together.
Isn't this temporary folder name going to change?
I don't exactly know what it's based on, and how the CLI knows which target folder to keep using if we're taking advantage of cached objects

@silvanocerza
Copy link
Contributor Author

The folder name in the temp dir has this structure arduino-sketch-<md5hash>, the md5 is calculated from the absolute path of the compiled sketch so if you move the sketch or rename it you should compile it again otherwise upload fails.
The --clean flag just deletes all existing files in the build directory so I don't expect it to break anything.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants