Skip to content

Conversation

@stuartmorgan-g
Copy link
Contributor

Enables the new end-to-end Linux desktop plugin tests.

Pre-launch Checklist

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I read the Tree Hygiene wiki page, which explains my responsibilities.
  • I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
  • I signed the CLA.
  • I listed at least one issue that this PR fixes in the description above.
  • I updated/added relevant documentation (doc comments with ///).
  • I added new tests to check the change I am making, or this PR is test-exempt.
  • All existing and new tests are passing.

Enables the new end-to-end Linux desktop plugin tests.
@stuartmorgan-g
Copy link
Contributor Author

@godofredoc do you have any pointers on the

[plugin_test_linux] [STDOUT] stderr: [        ]     error: downloading 'https://github.com/google/googletest/archive/release-1.11.0.zip' failed
[plugin_test_linux] [STDOUT] stderr: [        ]          status_code: 1
[plugin_test_linux] [STDOUT] stderr: [        ]          status_string: "Unsupported protocol"
[plugin_test_linux] [STDOUT] stderr: [        ]          log:
[plugin_test_linux] [STDOUT] stderr: [        ]          --- LOG BEGIN ---
[plugin_test_linux] [STDOUT] stderr: [        ]          Protocol "https" not supported or disabled in libcurl

error here? Do I just need to add the curl dependency in .ci.yaml for this task that I see in some other tasks?

@stuartmorgan-g
Copy link
Contributor Author

@godofredoc Ping on the question above; is a config change all that's needed to make curl work?

@godofredoc
Copy link
Contributor

o I just need to add

Yes, let's add it as a dependency to the .ci.yaml file. It will be added automatically to the path. If it doesn't work please ping me a build link to help triage.

@stuartmorgan-g stuartmorgan-g changed the title Enable plugin_tests_linux Enable plugin_test_linux Mar 2, 2023
stuartmorgan-g added a commit that referenced this pull request Mar 2, 2023
Speculative fix for the failure discussed in #120912
@stuartmorgan-g
Copy link
Contributor Author

@godofredoc It's still failing with the same error, unfortunately: https://ci.chromium.org/ui/p/flutter/builders/try/Linux%20plugin_test_linux/2/overview

Any idea how we can make this work? I haven't seen a failure like this outside of LUCI.

@godofredoc
Copy link
Contributor

@keyonghan FYI

@keyonghan
Copy link
Contributor

Will take a look after a P0 issue.

@keyonghan
Copy link
Contributor

I can verify that the curl package is installed correctly. Tried curling the referenced url directly as a separate step, it works. See step 8.
https://ci.chromium.org/raw/build/logs.chromium.org/flutter/led/keyonghan_google.com/4d04910bc7cde441d0cb17572fd7ffe0a52eff4755ef68cc86c90eac33598208/+/build.proto?server=chromium-swarm.appspot.com

@stuartmorgan Do you know how the curl command is executed from the test? By searching around, the Unsupported protocol error happens when things like inappropriate quotes exist.

@stuartmorgan-g
Copy link
Contributor Author

stuartmorgan-g commented Mar 7, 2023

It's run by CMake as part of the build; see https://github.com/flutter/flutter/blob/master/packages/flutter_tools/templates/plugin/linux.tmpl/CMakeLists.txt.tmpl#L65-L69

Looking around a bit more online now that I know it's only happening with CMake, I found some discussion suggesting that it may have to do with how the installed version of CMake was built and what libcurl it links to. Is this a version we built ourselves?

@keyonghan
Copy link
Contributor

keyonghan commented Mar 7, 2023

@stuartmorgan-g
Copy link
Contributor Author

That's just a path mistake in the test code; I can update this PR to fix that tomorrow. Thanks for tracking down the curl problem!

- bin/**
- .ci.yaml

- name: Linux plugin_test_linux
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

@stuartmorgan-g stuartmorgan-g Mar 8, 2023

Choose a reason for hiding this comment

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

Great! That has to be a separate PR that lands and then propagates I assume, like other ci.yaml changes?

Copy link
Contributor

Choose a reason for hiding this comment

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

Presubmit CI will pick up the changes right away, so changes in this same PR will work.

Please allow https://flutter-review.googlesource.com/c/recipes/+/40060 to land before pushing the change, as the infra config complains about the special character @ in the version: https://ci.chromium.org/ui/p/flutter/builders/try/Linux%20ci_yaml%20flutter%20roller/2119/overview.

After the above CL lands, please use version build_id:8787856497187628321 instead of version:[email protected].
(successful led run with version build_id:8787856497187628321: :https://ci.chromium.org/raw/build/logs.chromium.org/flutter/led/keyonghan_google.com/9a8f995890c8aa1dd97b86de8a5f3a2018dcce8050fcc2f2143d6b6569aa0e5c/+/build.proto?server=chromium-swarm.appspot.com)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Presubmit CI will pick up the changes right away, so changes in this same PR will work.

Ah, nice. Is there a list somewhere of what does and doesn't work in-PR? I know that timeout changes don't take effect right away, for instance, having been bitten by that before, but I'm not clear on what things do.

Copy link
Contributor

Choose a reason for hiding this comment

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

All properties should be picked up right away. Updated flutter/cocoon#2519 to make it clear.

@keyonghan
Copy link
Contributor

cmake was bundle updated in #122147. A rebase should green the CI.

Copy link
Contributor

@keyonghan keyonghan left a comment

Choose a reason for hiding this comment

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

LGTM

@stuartmorgan-g
Copy link
Contributor Author

Thanks for all the help with resolving the blocker!

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 10, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 10, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 10, 2023
hannah-hyj pushed a commit to hannah-hyj/flutter that referenced this pull request Mar 11, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 11, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 12, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 10, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

autosubmit Merge PR when tree becomes green via auto submit App

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants