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

always build devtools from a specified, specific sdk version #3216

Merged
merged 6 commits into from Jul 28, 2021

Conversation

devoncarew
Copy link
Member

always build devtools from a specified, specific sdk version:

  • rev the version of flutter we use to the latest dev release (2.4.0-4.1.pre)
  • in the build_release.sh script, validate that we're building with the version specified in flutter-version.txt
  • add a new tool/update_flutter_sdk.sh script that can 1) clone the flutter sdk if it doesn't exist, or 2) switch the vended flutter sdk to be the version specified in flutter-version.txt. Developers can just run tool/update_flutter_sdk.sh whenever sdk version changes in order to make sure they're on the correct sdk

Some changes here are a bit proscriptive, and will affect the workflows of people working on this repo, so comments welcome. cc @kenzieschmoll @jacob314 @terrylucas @DanTup

@devoncarew
Copy link
Member Author

I may also need to update this line as well: https://github.com/flutter/devtools/blob/master/.github/workflows/build.yaml#L14.

@DanTup
Copy link
Contributor

DanTup commented Jul 26, 2021

I may also need to update this line as well: https://github.com/flutter/devtools/blob/master/.github/workflows/build.yaml#L14

I suspect you can delete that, as it looks like the one being set in the environment is coming from the matrix:

        env:
          BOT: main
          CHANNEL: ${{ matrix.channel }}

If there are other jobs relying on the env variable that don't explicitly have the above, it could be added. That would save having the version in an additional place.

The changes lgtm, but doesn't some of this code run using the users version of Dart (eg. the server code and resolution of dependencies), so is there still value in having some tests run with things like the stable branch? Eg. with this change, could I easily update the repo so that pub global activate devtools would fail with the current stable version of Dart/Flutter but the bots would all be passing?

(this might not matter if editors migrate away from the server/pub versions to SDK-provided versions, but I'm not sure if that's imminent).

@DanTup
Copy link
Contributor

DanTup commented Jul 26, 2021

I suspect you can delete that, as it looks like the one being set in the environment is coming from the matrix

Actually, I realise there are multiple jobs in this file now, and that matrix only applies to main, so maybe it is required.

Although my comments above are probably relevant even without this change, if the existing bots are all only running on dev but users may be activating from stable?

echo "flutter-version.txt != flutter-sdk/version"
echo " $REQUIRED_FLUTTER_VERSION != $ACTUAL_FLUTTER_VERSION"
echo ""
echo "To switch versions, run './tool/update_flutter_sdk.sh'."
Copy link
Member

Choose a reason for hiding this comment

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

instead of failing if we are on the wrong version, can update_flutter_sdk.sh be ran automatically as part of this build release script?

Copy link
Member Author

Choose a reason for hiding this comment

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

We could, but I think it's useful for developers to be aware that they need to be building / running with the same SDK as what we ship with. I.e., prompting them to run tool/update_flutter_sdk.sh is probably a good reminder about how to keep your local version synced.

@devoncarew
Copy link
Member Author

I suspect you can delete that, as it looks like the one being set in the environment is coming from the matrix

Actually, I realise there are multiple jobs in this file now, and that matrix only applies to main, so maybe it is required.

Although my comments above are probably relevant even without this change, if the existing bots are all only running on dev but users may be activating from stable?

I'm mostly concerned with ensuring that we're testing against the same version that we're building for the web front-end - that's the bulk of the tool. It's useful - but less critical - to validate the cli against multiple sdk versions.

In the limit devtools will be in lock-step with a sdk version, so we won't have to guess about what we're running on.

@devoncarew
Copy link
Member Author

devoncarew commented Jul 27, 2021

I'm going to re-work this PR a bit to make it clearer with tests / builds are using what SDKs.

env:
CHANNEL: dev
CHANNEL: 2.4.0-4.1.pre
Copy link
Contributor

Choose a reason for hiding this comment

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

(There's a 2.5.0-1.0.pre dev now, not sure if it's worth bumping again before landing?)

@devoncarew devoncarew merged commit 736fa38 into flutter:master Jul 28, 2021
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.

None yet

3 participants