More commandline improvements #2000
Conversation
Previously, two separate booleans (doUpload and doVerify) were used. However, since it always makes sense to specify only one of them, it makes more sense to keep a single action enum variable, which slightly simplifies the code (especially when more actions are added later). Additionally, an error is now shown when both --verify and --upload are specified on the commandline.
Previously, the code showed an error when the given action was not upload or verify. This is now reversed: the GUI is shown when the action is "GUI" (which is the default when no action specified). Since the action enum only contains these three values, there is no change in behaviour, but this makes it easier to add new actions later.
This uses a switch on the action value, which makes it more clear what code runs when. No actual behaviour is changed, most of the changes in this commit are indentation changes.
This method takes care of setting the serial.port preference to the given value, as well as deriving the serial.port.file preference. This should prevent duplicate code in the future. Note that a second copy of this code lives in SerialUploader, but that doesn't write to the global Preferences but a local prefs map. Since the global Preferences are currently static, there is no way to share code between these two copies.
This describes the versions where various options were introduced or changed.
Previously, the --board and --port arguments were stored in a variable first and only processed later. Now, the arguments are processed right away. This does mean that the arguments are processed when the GUI is not yet initialized, which caused problems with calling onBoardOrPortChange and friends from selectBoard. However, since the GUI is not initialized, there is no real reason to call them either - if we just set the preferences to the right values, the GUI will be initialized correctly later. For this reason, selectBoard no longer calls the GUI update methods. Instead, those are called from the GUI code when the board is changed through the menu instead (e.g., after calling selectBoard). This commit slightly changes behaviour. Previously, --board and --port only worked in combination with --verify and --upload, but were ignored when just starting the IDE. Now, these are processed regardless of the other options present. Additionally, this commit causes all changed preferences to be saved. Previously, only changes with --pref were saved, --board and --port options were only active for the current run. This was caused because the saving of the preferences happened as a side effect of loading the file in the Editor, but only the --pref option was processed at that time. Note that the --verbose options are still only active for the current run and are only valid combined with --verify or --upload (since they default to non-verbose instead of the current preference).
Since the handling of these options defaults to non-verbose (instead of the current preference), they make no sense when starting the IDE normally. Previously, these options would just be ignored in this case, now an error is shown.
Before, the preferences were saved as a side effect of loading files in the Editor, but it seems better to explicitely save them as well (this should prevent problems later on, if the Editor class is no longer used in --verify or --upload mode).
This allows setting preferences for the current run only, without remembering them for the next run. This is especially useful when combined with --verify or --upload.
Preferences.init would write out the default preferences when no preference file previously existed. This would cause a default preferences file to be written even when --no-save-prefs was passed, due to the ordering of things. However, since the Base constructor now already calls Preferences.save(), there is no need for Preferences.init to also do this. Since Base calls this after parsing the commandline, the --no-save-prefs option is now also properly respected.
Previously, --verbose would be processed after the preferences were saved, which should usually mean that it should never influence the saved preferences. However, if for whatever reason Preferences.save() would be called later, the verbosity preferences would still be messed up. Since we now have a Preferences.setDoSave() method, we can make sure that these verbosity preferences (and any other preferences that are changed after the build started) are never saved.
This option causes the IDE to process its commandline arguments and then quit. This allows setting preferences uses --pref, without having to also load the GUI or compile a sketch.
This allows reading specific preferences from the commandline.
Parsing commandline arguments inside Preferences isn't very elegant, this is better suited for the main function. Also, this change prepares for taking --curdir into account for --preferences-file as well.
Previously, the argument to --preferences-file would be interpreted as a filename, but then also checked as an option as well (in the next loop iteration). This didn't really matter in practice (unless you would be using a file called "--preferences-file"), but better skip the argument anyway.
This shouldn't change any behaviour, but prepares for upcoming changes.
|
I just added one more commit, that makes sure errors are actually printed to stderr too (previously, errors passed to |
|
I added two more commits improving the handling of the I want to go over the entire codebase to see if there are filenames from other preferences or sources that also need to take into account curdir, I'll add a commit for those later. |
This saves a few conversions from File object to String and is generally cleaner.
This method takes filenames as specified on the commandline and turns them into the right File object, taking into account the current directory passed through --curdir by the wrapper script.
On Windows, files are canonicalized to prevent issues with legacy 8.3 filenames. However, this canonicalization includes making the path absolute and this happened before applying --curdir to the path, making the latter a noop. By reversing the operations, this should allow both of them to do their work.
When a sketch looks like this:
Blink/
Blink.ino
Foo.ino
The idea is that opening Foo.ino should open up the sketch. However,
before this would show an error stating "The file Foo.ino needs to be
inside a sketch folder named Foo" instead.
This turned out to be due to a typo, which seems to have been present
for a long time. Note that when the main sketch file was a .pde file,
everything already worked as expected.
In a lot of places, (potentially) relative paths were passed to File without any processing, making them be resolved without taking into account --curdir. By passing them through Base.absoluteFile instead, these paths are resolved relative to the working directory before starting arduino (at least on Linux, which is currently the only platform supporting --curdir). This applies --curdir to the --preferences-file option and the build.path, settings.path, sketchbook.path preferences. For example, this now works as expected: arduino --pref build.path=build_dir --verify Blink.ino
Before, these were only shown in the GUI, which makes a failing commandline build a bit puzzling. As a side effect, the error is now shown in the log area in addition to the status line above the log area, but that should be ok.
When no build.path preference is present, a temporary directory is automatically created (and deleted). When a build.path was specified, but the directory does not exist, the IDE would show an error and fail to build, which is unexpected and not so friendly. This commit makes sure that the build directory is automatically created.
|
Ok, as promised I added some more relative path handling. I ended up squashing all three relative path handling commits together instead of adding one more. Furthermore, I added some more |
In the preferences dialog, the name of the preferences file is shown for advanced editing. If the filename is clicked, the folder containing the file is opened. However, this always used Base.getSettingsFolder, which is the folder where the settings file _normally_ resides. But when the --preferences-file option is used, the actual preferences file might be somewhere else. This commit makes sure to always open up the parent directory of the actual preferences file in use, instead of always the default one.
|
the patch looks good overall, I have only one concern: previously the default command-line behaviour was to NOT save the preferences. This patch changes this and it may be unexpected for users already using the IDE from command-line. Would it be better to change it back to not save by default and enable saving only with "--save-prefs"? Finally, I would ask you if you can rebase the patch on the current ide-1.5.x branch so I can merge this with a cleaner history. |
|
IIRC the changes would be saved in some circumstances and not in others (or rather,
Which contradicts what you say. I don't have time right now to look more closely. I'll rebase it later (though that might be after my vacation, which starts next monday and lasts 2 weeks). |
Uhm... ok I'll check that.
Oh, don't worry then, I'll take care to rebase the pull requst! |
|
The commit message of bdc98a9 says:
which suggests that the Editor class already made sure that preferences passed to --pref were saved. Process some commandline arguments earlier Furthermore 34f5930 says:
So, the handling of Does that help? :-) |
Yeah, you beat me on that... :) In a way I like the fact that the
So, I'd like to change the default to not save preferences (and change |
|
Sounds reasonable. However, what happens when you pass |
Now I understand why you added a Thinking on that a bit more, maybe the GUI should start only if none of the following options is used:
|
|
Not sure why anyone would want that :-) Perhaps to have two shortcuts to quickly toggle between board settings. It's largely hypothetical, though. I'm not sure I like your proposal much, it removes flexibility. What about adding Did you intentionally leave out options from your list? |
Yeah, just forget about them, they would be part of the list.
That could be a compromise, in this case we can just rename your
Indeed we lose flexibility, but those are weird corner cases that are now only accidentally allowed. When I initially introduced the command-line mode these use cases wasn't contemplated at all. |
Well, right now, the use of |
You're right, and the Here a table to recap the whole discussion (and to help my understanding :)):
The only case where
I'm missing something else? |
Ah, good point. Since You also make a good point about when There is only once other case for which we're dropping support by doing this (and I think it is supported with my code above), is starting the GUI with |
👍
I guess we can easily ignore this case. Ok I think we finally got a solution, I'll try to implement this one tomorrow on top of this pull request (unless you want to give it a spin of course). |
|
Feel free to take this one up, I'm a bit busy with other things this week :-) |
See arduino#2000 for details. Close arduino#2000
|
Implemented in #2253 |
This pullrequest further improves the Arduino commandline and manpage and contains some small cleanups as well. The initial incentive for this patch series was making the saving of preferences consistent and adding the
--no-save-prefsoption, but I found some other things along the way.The commit "Fix --curdir on Windows" has not been tested, since I don't have a Windows build environment set up.