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

[engine] Linux Fuchsia and Linux Fuchsia FEMU don't have runif logic #93261

Closed
CaseyHillers opened this issue Nov 8, 2021 · 17 comments · Fixed by flutter/engine#45134
Closed
Assignees
Labels
c: new feature Nothing broken; request for a new capability dependency: fuchsia Fuchsia team may need to help us engine flutter/engine repository. See also e: labels. P3 Issues that are less important to the Flutter project team Infra upgrades, team productivity, code health, technical debt. See also team: labels. team-engine Owned by Engine team triaged-engine Triaged by Engine team

Comments

@CaseyHillers
Copy link
Contributor

https://github.com/flutter/engine/blob/master/.ci.yaml#L160-L176

This causes these builds to always be run on presubmit, even if the change was just a markdown.

See flutter/engine#29566 where these were run on a darwin only change.

Adding some paths in the runif for where Fuchsia code is would help prevent unnecessary testing

@CaseyHillers CaseyHillers added engine flutter/engine repository. See also e: labels. dependency: fuchsia Fuchsia team may need to help us labels Nov 8, 2021
@CaseyHillers
Copy link
Contributor Author

\cc @chandarrengoog

@dnfield
Copy link
Contributor

dnfield commented Nov 8, 2021

I started drafting a runIf block for Fuchsia and it looks like this:

      - .ci.yaml
      - BUILD.gn
      - DEPS
      - assets/**
      - benchmarking/**
      - build/**
      - ci/**
      - common/**
      - flow/**
      - flutter_frontend_server/**
      - fml/**
      - lib/**
      - runtime/**
      - shell/common/**
      - shell/gpu/**
      - shell/platform/BUILD.gn
      - shell/Platform/config.gni
      - shell/platform/common/**
      - shell/platform/fuchsia/**
      - shell/platform/embedder/**
      - testing/**
      - version/**
      - vmservice/**
      - sky/**
      - third_party/tonic/**
      - third_party/txt/**
      - tools/const_finder/**
      - tools/font-subset/**
      - tools/fuchsia/**

Which seems kind of brittle and scary to me. It's basically saying don't run if you changed files that are only under shell/platform/(darwin|android|linux|windows|glfw). Maybe there's some better way to express that?

Ideally we'd be using something like gn analyze to figure out whether the git ls-files changed anything that's part of the target(s) we plan to build, and ideally before scheduling the bot.

@dnfield dnfield added P3 Issues that are less important to the Flutter project c: new feature Nothing broken; request for a new capability team Infra upgrades, team productivity, code health, technical debt. See also team: labels. team-infra Owned by Infrastructure team labels Nov 8, 2021
@chandarrengoog
Copy link
Contributor

chandarrengoog commented Nov 8, 2021

CC: @arbreng maybe you would have a better idea?

@arbreng
Copy link
Contributor

arbreng commented Nov 8, 2021

This seems like a pretty good way to free up some CI resources, nice catch :)

@zanderso
Copy link
Member

zanderso commented Dec 1, 2021

@arbreng It sounds like @dnfield's suggestion could be pretty brittle. Do you know of any more robust way of doing this?

@arbreng
Copy link
Contributor

arbreng commented Dec 2, 2021

I like this suggestion: "Ideally we'd be using something like gn analyze to figure out whether the git ls-files changed anything that's part of the target(s) we plan to build, and ideally before scheduling the bot."

The fuchsia embedder uses all of the engine, just like the other embedders do. So, this " It's basically saying don't run if you changed files that are only under shell/platform/(darwin|android|linux|windows|glfw)." is the correct pattern.

Darwin would have " It's basically saying don't run if you changed files that are only under shell/platform/(android|fuchsia|linux|windows|glfw)." and the same goes for the other embedders.

@godofredoc
Copy link
Contributor

Closing because the bug has not been updated for more than one year.

Feel free to reopen any bugs that you believe are important but please provide updates on how to resource the fix of the bug and what can be done to prevent the issue from not getting updates for another year.

@zanderso
Copy link
Member

This is still a valid issue. Reopening.

@zanderso zanderso reopened this Jun 20, 2023
@godofredoc
Copy link
Contributor

Removing infra-team as it lacks the context to define the right filters and filtering is already supported in the .ci.yaml file.

@godofredoc godofredoc removed the team-infra Owned by Infrastructure team label Jun 20, 2023
@dnfield
Copy link
Contributor

dnfield commented Jun 20, 2023

@godofredoc - based on my question in #93261 (comment), does ci.yaml support some way of saying "run if not"?

The problem for this target is that it should not be run if the changes are exclusive to a few directories. If we craft a regular "run-if" it will regress over time when someone adds a new directory.

@godofredoc
Copy link
Contributor

The filtering logic runs server side and is limited to use the pull request data returned by github.

Not sure I understand how the "run if not" will be used, @dnfield can you please provide an example to see if it can be calculated from the data available to backend?

@dnfield
Copy link
Contributor

dnfield commented Jun 20, 2023

Right now we can say:

"Run if any files have changed in the following directories."

I want to be able to say:

"Run if there are any changes to files not in the following directories"

@flutter-triage-bot flutter-triage-bot bot added team-engine Owned by Engine team triaged-engine Triaged by Engine team labels Jul 8, 2023
@godofredoc godofredoc self-assigned this Aug 22, 2023
godofredoc added a commit to godofredoc/cocoon that referenced this issue Aug 22, 2023
This is required to filter fuchsia changes in presubmit.

Bug: flutter/flutter#93261
auto-submit bot pushed a commit to flutter/cocoon that referenced this issue Aug 24, 2023
This is required to filter fuchsia changes in presubmit.

Bug: flutter/flutter#93261
@godofredoc
Copy link
Contributor

@dnfield run_if_not has been implemented moving this bug back to you for the final filters.

@godofredoc godofredoc assigned dnfield and unassigned godofredoc Aug 24, 2023
@zanderso
Copy link
Member

zanderso commented Aug 25, 2023

@matanlurey @johnmccutchan This is relevant to our conversation the other day.

@matanlurey
Copy link
Contributor

I'm happy to take this issue.

@matanlurey matanlurey assigned matanlurey and unassigned dnfield Aug 25, 2023
matanlurey added a commit to flutter/engine that referenced this issue Aug 29, 2023
…s. (#45134)

Closes flutter/flutter#93261.

I probably could have been more clever with a regular-expression but
this seems easier to read.

---------

Co-authored-by: godofredoc <godofredoc@google.com>
@zanderso
Copy link
Member

Thanks, Matan!

gaaclarke pushed a commit to gaaclarke/engine that referenced this issue Aug 30, 2023
…s. (flutter#45134)

Closes flutter/flutter#93261.

I probably could have been more clever with a regular-expression but
this seems easier to read.

---------

Co-authored-by: godofredoc <godofredoc@google.com>
@github-actions
Copy link

This thread has been automatically locked since there has not been any recent activity after it was closed. If you are still experiencing a similar issue, please open a new bug, including the output of flutter doctor -v and a minimal reproduction of the issue.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 12, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
c: new feature Nothing broken; request for a new capability dependency: fuchsia Fuchsia team may need to help us engine flutter/engine repository. See also e: labels. P3 Issues that are less important to the Flutter project team Infra upgrades, team productivity, code health, technical debt. See also team: labels. team-engine Owned by Engine team triaged-engine Triaged by Engine team
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants