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

regression: fixed ARDUINO_USER_AGENT pollution / resolve compile-sketch GH action failure on Teensy #2669

Merged
merged 3 commits into from
Jul 19, 2024

Conversation

cmaglie
Copy link
Member

@cmaglie cmaglie commented Jul 18, 2024

Please check if the PR fulfills these requirements

See how to contribute

  • 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)
  • UPGRADING.md has been updated with a migration guide (for breaking changes)
  • configuration.schema.json updated if new parameters are added.

What kind of change does this PR introduce?

Arduino CLI sets the environment variable ARDUINO_USER_AGENT with the currently running version and passes this variable down to the tools (compilers, uploaders, etc.). This PR fixes a regression in the user agent string.

What is the current behavior?

In the official release build ARDUINO_USER_AGENT is set as arduino-cli/1.0.2 gRPCClientUnknown/0.0.0.
A build from source via task build sets ARDUINO_USER_AGENT as arduino-cli/git-snapshot gRPCClientUnknown/0.0.0.

What is the new behavior?

The new official release build will set ARDUINO_USER_AGENT as arduino-cli/1.0.3.
A build from source via task build now sets ARDUINO_USER_AGENT as arduino-cli/git-snapshot.
git-snapshot has been replaced by 1.0.0-snapshot to comply with semver specification. This change also reflects that the snapshot has been built from the 1.x.x series branch.

Does this PR introduce a breaking change, and is titled accordingly?

No

Other information

This PR also fixes #2658. The Teensy tools check for the ARDUINO_USER_AGENT variable, in particular, it seems to be very picky about the arduino-cli/x.x.x part (it must be a strict version >=0.35.0) and there must be no extra parts.

This is a regression starting from 1.0.0.

@cmaglie cmaglie self-assigned this Jul 18, 2024
@cmaglie cmaglie added topic: code Related to content of the project itself type: imperfection Perceived defect in any part of project labels Jul 18, 2024
@cmaglie cmaglie force-pushed the fix_user_agent_regression branch 2 times, most recently from ace211b to 8d07cda Compare July 18, 2024 08:26
Copy link

codecov bot commented Jul 18, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 70.26%. Comparing base (8246eaa) to head (aae8c8a).

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2669      +/-   ##
==========================================
+ Coverage   70.22%   70.26%   +0.03%     
==========================================
  Files         222      222              
  Lines       21346    21343       -3     
==========================================
+ Hits        14991    14997       +6     
+ Misses       5176     5170       -6     
+ Partials     1179     1176       -3     
Flag Coverage Δ
unit 70.26% <ø> (+0.03%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@alessio-perugini alessio-perugini left a comment

Choose a reason for hiding this comment

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

A few questions:

  • Is 1.0.0-snapshot clear enough? Maybe 1.0.0-git-snapshot, 1.0.0-snapshot.git, and 1.0.0-snapshot+git can be clearer?
  • At this point should we change the nightly as well? Like 1.0.0-nightly?

To be honest it's confusing to see this 1.0.0 prefix for nightly and snapshot builds. I'll probably live as it was.
Shouldn't the 968689f alone fix the #2658?

@cmaglie
Copy link
Member Author

cmaglie commented Jul 18, 2024

To be honest it's confusing to see this 1.0.0 prefix for nightly and snapshot builds. I'll probably live as it was.
Shouldn't the 968689f alone fix the #2658?

I tried to run into a CI by using the example given by @per1234 here #2658 (comment), since it builds the CLI from a git ref, the version reported in the user agent is git-snapshot and it will trigger the bug in #2658. -> https://github.com/cmaglie/cli-action-test/actions/runs/9980439654

BTW It's also true that the compile-sketch action will run only tagged versions, so it's impossible to run a build with an invalid semver like git-snapshot or nightly. This also means that running a build of the CLI from a task build or a nightly will always trigger the execution of the Teensy tool.

Said that, given the pro and cons, I'm positive to leave git-snapshot as it was.

@alessio-perugini
Copy link
Contributor

@cmaglie ok, I'm more keen to leave the git-snapshot as it is. Because, as you said, the compile-sketch action runs only tagged versions, changing that for the purposes of making Teensy tool work when manually building, feels like something that should be addressed from their side.

Copy link
Contributor

@umbynos umbynos left a comment

Choose a reason for hiding this comment

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

I like more git-snapshot too

@cmaglie cmaglie merged commit eb28638 into master Jul 19, 2024
200 checks passed
@cmaglie cmaglie deleted the fix_user_agent_regression branch July 19, 2024 12:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic: code Related to content of the project itself type: imperfection Perceived defect in any part of project
Projects
None yet
Development

Successfully merging this pull request may close these issues.

arduino-cli v1.0.x breaks arduino/compile-sketches action for Teensy boards
3 participants