Fixes related to commandline options #1639
Conversation
|
Oh, these commits use showError(), which always sets the exit code to 1. I just noticed the documentation promises to return 2 when the sketch was not found, so this might need some refactoring as well... |
|
I just updated this pullrequest. I rebased on top of the latest ide-1.5.x branch and added a few more changes. |
Since the custom suboptions are only visible when their associated board is the currently selected one, there is no point in re-setting the current board when a suboption is selected.
Previously, strings containing the board id, or a joined version of the package, platform and board id were passed around. Since comparing objects is easier than strings and since parsing strings can be fragile, it's better to just pass the TargetBoard objects. There is one case where string parsing is still required: when parsing the --board commandline option. However, the parsing is now done in the right place, when handling the commandline, instead of in a generic selectBoard method.
Previously, it would just raise nullpointer or index out of bounds exceptions when the --board paramater was wrong.
Previously, this would error out with an index out of bounds exception. Now, an IOException is thrown, which is properly handled further up the call chain.
Instead of opening up files during argument processing, the filenames are now stored in a list and opened only after all commandline arguments have been processed. This commit in itself shouldn't change any behaviour, but it prepares for improved error reporting in the next commits.
Previously, any files that were specified on the commandline but could not be opened were silently ignored. Only if --verify and --upload was specified and _all_ files failed to open, a generic error message was shown. Additionally, if multiple files were specified with --verify or --upload, only the first would be acted on (the others would be openened and shown in the GUI, but not actually verified or uploaded). Now, whenever a file fails to open, an error message is shown (fatal with --verify or --upload, non-fatal otherwise). Furthermore, with --verify or --upload an error is shown when there is not exactly one file on the commandline. Finally, instead of keeping an "opened" variable, the code now just checks the size of "editors" to see if a blank sketch should be opened.
This allows setting preferences through the commandline.
Previously, these arguments would be passed to the compile method. However, passing them to the constructor makes sure that the build preferences are created sooner, so they can be used by Sketch before calling the compile method. This commit shouldn't change any behaviour, but prepares for the next commits.
Instead of defining in the preprocess method and returning, just define it in the build method. This makes sure the name is available before preprocessing, which is important for the upcoming commits. This commit should not change behaviour, only prepare for the next commits.
Previously, a full cleanup of the work directory (and thus a full rebuild) was done on the first build after: - startup, or - a change in the board or board suboption. This did not cooperate nicely with commandline compilation using --verify. Using the build.path option a persistent build path could be used, but the actual files in that path would never be reused. Now, each build saves the preferences used for building in a file "buildprefs.txt" inside the build directory. Subsequent builds will read this file to see if any build options changed and re-use the existing files if the build options are identical. Because the main .cpp file is not handled by Compiler::build, but by Sketch::preprocess, it is still always regenerated, even if the Sketch itself didn't change. This could be fixed later, though it is probably not a problem. When writing buildprefs.txt, only the build preferences starting with "build.", "compiler." or "recipes." are used. These should be enough to ensure files are always rebuilt when needed (probably also sometimes when not needed, when change build.verbose for example). Using all build preferences would cause the files to be rebuild too often, and because of last.ide.xxx.daterun, they would still rebuild on _every_ invocation... This approach is perhaps not ideal, but improving it would require putting more structure in the preferences instead of piling them all together into the build preferences. Because of this new mechanism, the old buildSettingsChanged()/deleteFilesOnNextBuild could be removed.
When System.(out|err).print was used before there was a visible EditorConsole, the message was written to the stderr/stdout by this instead of the EditorConsole. However, the write(data, offset, length) version would not pass on its offset and length parameters to the stdout/stderr stream, causing (parts of) a message to be printed multiple times. This commit makes sure the parameters are all properly passed to the real stream. For some reason the write(int) and write(byte[], int, int) methods in PrintStream do not throw an IOException like the write(byte[]) version, so the try block has to go.
These are intended to be ran from the commandline, so showing the GUI doesn't make so much sense. This is not quite the perfect solution yet, because an Editor object and all kinds of GUI objects are still created. This commit only prevents them from being visible, which is a nice first step, but not quite pretty yet. However, to do it properly, some code should be moved out of the Editor class, so that's a bit more work. Additionally, any messages shown with Base::showError and friends still create a popup, they probably shouldn't do this either.
Previous commits made all failures return 1, even though originally an unknown sketch file would return 2. This restores the previous behaviour and adds return code 3 to mean invalid options specified. The return codes are now: 0: Success 1: Build failed or upload failed 2: Sketch not found 3: Invalid commandline options
|
I updated the pulrequest again. I made some minor corrections and added one more commit to fix the exit codes again. These commits add somet translatable strings (passed through the _ function). Is there anything I need to do for those? As far as I'm concerned, this pullrequest is now ready to merge. There's still more stuff to improve, but this should provide a solid base and at least make the commandline interface usable. After it is merged, I'm happy to update the wiki page with more examples and documentation. |
|
I've reviewed the commits and all seems good to me (also the "incremental" style that you apply to your pull requests helps a lot on that). Thanks! I'll take care to update translations on transifex and pull the translated resources back into the repository before the next release, don't worry about that. C |
|
@matthijskooijman |
|
Could you update the wiki, so it explains that on Mac you should open the application and run JavaApplicationStub to use the command line? |
|
@Lauszus, Uh, could you be bit more verbose about that? I'm not quite familiar with MacOS and what you're saying doesn't quite make sense to me yet :-) |
|
Of course :) Here you will find the executable file called "JavaApplicationStub". This can then be executed just like "arduino" on Linux ;) I am not sure if it got any limitations compared to the Linux version. |
|
Hmm I found one warning message a bit annoying: Maybe this should only be shown in verbose mode? Or perhaps only if it can not find the current board selected to give an indication of what might be wrong? |
|
@Lauszus, what is this Dropbox/Arduino/hardware/ folder? I think the IDE expects 3rd party hardware specs in there, but you just have a some other code in there? Perhaps you should just rename that folder? |
|
It is hardware add-ons, but they are not compatible with 1.5. In fact my Sanguino add-on support both - see: https://github.com/Lauszus/Sanguino - note that it actually doesn't produce an warning as it finds it in the end. I have some other ones as well which ONLY support 1.5 and they of course don't produce any warnings. |
|
Maybe just remove all the |
|
@cmaglie, while updating the documentation, I was struggling a bit with the markdown syntax. We're essentially writing a manpage there, but markdown doesn't support that very well. I'd like to switch to Asciidoc instead, which has better manpage support (and even can be converted to a real UNIX manpage). However, an additional thought occured to me: Why not put the asciidoc sources inside the git tree? And then during the build (at least for Linux), actually generate a manpage file? This would be also be useful for e.g., the offiical Debian package, which can then just ship the official documentation as a manpage. For online viewing of the documentation, we can just refer to the file in git, since github supports rendering asciidoc manpages on-demand (and I've checked that it actually looks decent as well). How does this sound? If good, then where would the manpage asciidoc be put? A new build/doc directory perhaps? Or build/shared/doc? |
|
@matthijskooijman did you know that you can clone the wiki too, just like any git repo. Here is the url: https://github.com/arduino/Arduino.wiki.git. Then it could just pull the wiki when doing a build. |
|
@Lauszus, that could work, but it would mean that the manpage generated when building a particular version of the IDE isn't tied to the IDE version. If you would build an older version of the IDE later, you'd get a manpage that is too new for the IDE you're building, which isn't very nice. Also, you'd add git as a build dependency, which is not currently the case I think. Given that, I think putting the manpage sources in the repo makes more sense... |
|
@Lauszus @matthijskooijman C |
|
Looks good. One thing I don't really like is the arguments getting parsed twice, the second one inside the Preferences class (IIRC). However, it seems that there is no easy fix for this (perhaps the argument parsing should be moved out of Base, and/or out of the Base constructor, but that'll probably have other side effects as well... |
|
Yeah, that deserve his own refactoring to have proper testing. Very low priority BTW, maybe one day... |
These mostly make the commandline a bit more robust. This also includes some refactoring to put the actual parsing code for the --board option in the right place.
I'll send a mail to the list in a minute with some additional throughts related to the commandline interface.