search

Arduino-CI / arduino_ci

Unit testing and Continuous Integration (CI) for Arduino libraries, from a Ruby gem
Apache License 2.0
110 stars 34 forks source link

Remove flag --no-dry-run for arduino-cli >= 0.14.0 #323

Closed hel06492 closed 1 year ago

hel06492 commented 2 years ago

Highlights from CHANGELOG.md

jgfoster commented 2 years ago

@hel06492, Thank you for this PR. Could you give us a bit of background on why it is desirable or what problem it solves? Next, why tie it to an arduino_ci version? If it is desirable, why not have it take effect immediately? Presumably people using the old version will already have the legacy behavior. Thanks!

hel06492 commented 2 years ago

@jgfoster The --dry-run flag was removed from version 0.14.0 and as for version 0.19.0 arduino-cli no longer accepts the flag. It fails with the following error. I can't find the changelog, only a forum post about the change.

Compiling Blink.ino for arduino:avr:mega:cpu=atmega2560... 
Last command:  $  /usr/bin/arduino-cli --format json compile --fqbn arduino:avr:mega:cpu=atmega2560 --warnings all --dry-run /home/salaryman/Arduino/trial.errors/Blink/examples/Blink/Blink.ino
Error: unknown flag: --dry-run

The version check is for older versions which might give unexpected behavior without the --dry-run flag.

Please let me know if there are more things I need to fix/add for this pull request. Thanks.

jgfoster commented 2 years ago

@hel06492, I think some of my confusion is that the change refers to arduino_ci (Continuous Integration) while it appears that it should refer to arduino_cli (Command Line Interface). Could you update the title and other text in the PR and comments in the code to make that clarification? Thanks!

ianfixes commented 2 years ago

Hi @hel06492, nice work on the ruby side of things! It looks like this relates to #277 👍

Please let me know if there are more things I need to fix/add for this pull request.

I only found one issue. In general, I would welcome contributions that upgrade DESIRED_ARDUINO_CLI_VERSION to the latest.

hel06492 commented 2 years ago

Hi @jgfoster @ianfixes Thanks for the suggestions. I've made changes according to those feedbacks.

ianfixes commented 2 years ago

@hel06492 Have you selected "allow edits from maintainers" on your PR? I can probably take care of the remaining issues.

hel06492 commented 2 years ago

@ianfixes yes, it's already checked

jgfoster commented 2 years ago

@ianfixes, I'm going through the open PRs and wonder if this can be merged. Thanks!

ianfixes commented 1 year ago

I've merged this with a larger body of work that will be merged as part of #334 -- thanks!

ianfixes commented 1 year ago

This has merged as part of #334 and was released in v1.4.0