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

🔥 Remove flutter as a submodule #9307

Merged
merged 11 commits into from
Sep 16, 2023
Merged

🔥 Remove flutter as a submodule #9307

merged 11 commits into from
Sep 16, 2023

Conversation

AlexV525
Copy link
Member

@AlexV525 AlexV525 commented Aug 26, 2023

Description of what this PR is changing or adding, and why:
The PR removes the flutter repo as a submodule to avoid conflicts when building the website on different platforms. It was first introduced in #1257 but with no explanation so we cannot track back what's the purpose, but removing this should not cause difficulties in maintaining the website.

Additional info

  • The build process is slower than the previous one because the checkout step needs an extra 3~5 minutes, depending on the network condition. See now and previous checks.

Issues fixed (partially) by this PR (if any):

Presubmit checklist

@github-actions
Copy link

github-actions bot commented Aug 26, 2023

Visit the preview URL for this PR (updated for commit 9347ffe):

https://flutter-docs-prod--pr9307-feat-remove-flutter-4mo6emc3.web.app

(expires Sat, 23 Sep 2023 20:19:43 GMT)

🔥 via Firebase Hosting GitHub Action 🌎

Sign: d5ba327eec813901cac8396c4f458b02288624ab

@atsansone atsansone added review.tech Awaiting Technical Review infra.structure Relates to the tools that create docs.flutter.dev labels Aug 26, 2023
@AlexV525
Copy link
Member Author

AlexV525 commented Aug 26, 2023

If we need flutter for local builds, we might consider adding it to the .gitignore too, and maybe with some more setup scripts.

@AlexV525
Copy link
Member Author

With the change, I was able to run make test locally on my Windows setup. But still not able to run make up because some symbolic link issues between platforms.

@parlough
Copy link
Member

parlough commented Aug 28, 2023

Thanks for working on this @AlexV525!

I haven't taken a close look yet, but I'll give it some thought tonight :)

Currently we benefit from dependabot updating the submodule for each stable Flutter release. This allows us to explicitly move to a new stable version of Flutter, rather than the build using whatever the stable branch happens to be at, potentially breaking the build with updates. In practice, this isn't usually an issue since we prepare for new stable builds in advance.

@AlexV525
Copy link
Member Author

This allows us to explicitly move to a new stable version of Flutter, rather than the build using whatever the stable branch happens to be at, potentially breaking the build with updates.

It can be done if we wrote the current ref we're using into configurations and use some dependabot mechanism to upgrade as I imagine.

Copy link
Member

@parlough parlough left a comment

Choose a reason for hiding this comment

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

Sorry about the delay on reviewing this. Thank you!

@parlough parlough merged commit 34a038b into main Sep 16, 2023
10 checks passed
@parlough parlough deleted the feat/remove-flutter branch September 16, 2023 20:42
atsansone added a commit to atsansone/website that referenced this pull request Sep 19, 2023
_Description of what this PR is changing or adding, and why:_
The PR removes the `flutter` repo as a submodule to avoid conflicts when
building the website on different platforms. It was first introduced in
flutter#1257 but with no explanation so we cannot track back what's the
purpose, but removing this should not cause difficulties in maintaining
the website.

### Additional info

- The build process is slower than the previous one because the checkout
step needs an extra 3~5 minutes, depending on the network condition. See
[now](https://github.com/flutter/website/actions/runs/5985413443/job/16237673762?pr=9307)
and
[previous](https://github.com/flutter/website/actions/runs/5983398408/job/16237673076?pr=9298)
checks.

_Issues fixed (partially) by this PR (if any):_
- flutter#6201
- flutter#6443
- flutter#8272

## Presubmit checklist

- [x] This PR doesn’t contain automatically generated corrections
(Grammarly or similar).
- [x] This PR follows the [Google Developer Documentation Style
Guidelines](https://developers.google.com/style) — for example, it
doesn’t use _i.e._ or _e.g._, and it avoids _I_ and _we_ (first person).
- [x] This PR uses [semantic line
breaks](https://github.com/dart-lang/site-shared/blob/main/doc/writing-for-dart-and-flutter-websites.md#semantic-line-breaks)
of 80 characters or fewer.

---------

Co-authored-by: Anthony Sansone <atsansone@users.noreply.github.com>
Co-authored-by: Parker Lougheed <parlough@gmail.com>
parlough added a commit that referenced this pull request Sep 19, 2023
…9416)

#9307 caused `dart` and `flutter`
to not be accessible when running inside the container. This PR attaches
the cloned Flutter repository as a volume to allow it to persist and be
accessible.

To be compatible with these changes, also modifies the `test.sh` script
to be simpler, not allowing switching branches separately from the
container. When it's ran from GitHub actions, a new container is created
anyway.
atsansone added a commit that referenced this pull request Sep 21, 2023
_Description of what this PR is changing or adding, and why:_
The PR removes the `flutter` repo as a submodule to avoid conflicts when
building the website on different platforms. It was first introduced in
#1257 but with no explanation so we cannot track back what's the
purpose, but removing this should not cause difficulties in maintaining
the website.

### Additional info

- The build process is slower than the previous one because the checkout
step needs an extra 3~5 minutes, depending on the network condition. See
[now](https://github.com/flutter/website/actions/runs/5985413443/job/16237673762?pr=9307)
and
[previous](https://github.com/flutter/website/actions/runs/5983398408/job/16237673076?pr=9298)
checks.

_Issues fixed (partially) by this PR (if any):_
- #6201
- #6443
- #8272

## Presubmit checklist

- [x] This PR doesn’t contain automatically generated corrections
(Grammarly or similar).
- [x] This PR follows the [Google Developer Documentation Style
Guidelines](https://developers.google.com/style) — for example, it
doesn’t use _i.e._ or _e.g._, and it avoids _I_ and _we_ (first person).
- [x] This PR uses [semantic line
breaks](https://github.com/dart-lang/site-shared/blob/main/doc/writing-for-dart-and-flutter-websites.md#semantic-line-breaks)
of 80 characters or fewer.

---------

Co-authored-by: Anthony Sansone <atsansone@users.noreply.github.com>
Co-authored-by: Parker Lougheed <parlough@gmail.com>
atsansone pushed a commit that referenced this pull request Sep 21, 2023
…9416)

#9307 caused `dart` and `flutter`
to not be accessible when running inside the container. This PR attaches
the cloned Flutter repository as a volume to allow it to persist and be
accessible.

To be compatible with these changes, also modifies the `test.sh` script
to be simpler, not allowing switching branches separately from the
container. When it's ran from GitHub actions, a new container is created
anyway.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
infra.structure Relates to the tools that create docs.flutter.dev review.tech Awaiting Technical Review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants