Change the root path of the license crawl to engine/src #177352
Conversation
|
It looks like this pull request may not have tests. Please make sure to add tests or get an explicit test exemption before merging. If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix? Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. If you believe this PR qualifies for a test exemption, contact "@test-exemption-reviewer" in the #hackers channel in Discord (don't just cc them here, they won't see it!). The test exemption team is a small volunteer group, so all reviewers should feel empowered to ask for tests, without delegating that responsibility entirely to the test exemption group. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the license crawl scripts to change the root path from engine/src/flutter to engine/src. This change is to support moving some third-party dependencies. The modifications include updating the working directory paths in shell scripts, prefixing paths in exclude.txt and include.txt with flutter/ to account for the new root, and adjusting the C++ license checker to hardcode the root package name to "flutter". The unit tests have been updated accordingly. The changes are consistent and correctly implement the intended refactoring. I have reviewed the changes and found no issues.
gaaclarke
left a comment
gaaclarke
left a comment
There was a problem hiding this comment.
The code looks good (modulo the hardcoded name). I'm a bit concerned with the approach because this is going to muddle up the audit logs. I thought about this a bit more. Here are some alternatives:
- Can you just add a symbolic link between the new location and the old location? Maybe the checker would have to be updated a bit to make sure that works.
- We could make our own symbolic link structure in //engine/src/flutter/tools/licenses_cpp/data/links that maps the 2 directories. Something like text files with 2 lines that specify the links source and destination?
The benefit there is that the folder we are scanning is more in line with what someone would have expected and we don't have to parse through tons of excluded files are come up with yet another way to exclude those files from being scanned.
| const std::array<std::string_view, 2> kThirdPartyIgnore = {"pkg", | ||
| "vulkan-deps"}; | ||
|
|
||
| const char* kRootPackageName = "flutter"; |
There was a problem hiding this comment.
Can you pipe this through to LicenseChecker::Flags, please? One of the big problems with the last license checker was that as people patched it they co-mingled data and logic piecemeal which made it hard to focus on fixing the actual logic. There is already an example there for you.
There was a problem hiding this comment.
done - moved the root package name to LicenseChecker::Flags
| ^tools/licenses_cpp/data/.* | ||
| ^build_overrides/.* | ||
| ^fuchsia/.* | ||
| ^out/.* |
There was a problem hiding this comment.
no action required: Unfortunately the way the license checker is designed is to be auditable. So even though we don't parse any of these files, just looking at them causes a cost in disk access and logs. This is going to make the logs much harder to audit.
There was a problem hiding this comment.
I added these filters out of caution to ensure that those directories would never be crawled. But from what I can tell they are not really necessary.
If I delete those three lines from exclude.txt, then when I run the license script it still produces the correct output. I did not see anything in the license checker's logs that mentions paths within out, fuchsia, or build_overrides.
Should I remove these lines from exclude.txt?
There was a problem hiding this comment.
Here's the output from above: https://logs.chromium.org/logs/flutter/buildbucket/cr-buildbucket/8700357321091203633/+/u/test:_Run_license_cpp/stdout
It didn't add a bunch of EXCLUDE: "out/ examples. I forgot that's because on CI we put our build outputs elsewhere. It did add a fair bit of EXCLUDE: "third_party/fuchsia-sdk but the execution time is the same.
Should I remove these lines from exclude.txt?
No, I think it's prudent to keep them. It won't affect CI runs but it will affect local runs.
Currently we do not check symlinks into the Git-managed parts of the engine tree. I'd like to maintain that property in order to simplify source control and ensure that the tree functions on platforms like Windows where symlinks may not be available. Projects that use GN/Ninja and other Chromium tools typically place third-party dependencies in |
| public: | ||
| struct Flags { | ||
| bool treat_unmatched_comments_as_errors = false; | ||
| std::string root_package_name = "flutter"; |
There was a problem hiding this comment.
Can you pipe this up to the command line flags please? That way when it's executed it looks like this:
licenses_cpp --path foo/bar --root-package-name=bar
Can you measure the time it takes to run the license checker locally before your change and after? I'm interested to see what the exclusion of the out/ directory is going to do to runtime (and the noise in the logs). If it is reasonable I'm fine with this approach for now and leave it as a potential cleanup in the future to allow out-of-tree inclusions instead of dropping the scanned directory one folder higher. I think that is the better solution.
There was a problem hiding this comment.
I'd make the root-package-name flag optional, where not supplying one keeps the old logic.
There was a problem hiding this comment.
Added a command line flag.
This PR does not significantly change the running time of the license checker.
Results for time ci/licenses_cpp.sh host_release on my Linux setup:
| With this PR | Without this PR |
|---|---|
| real 15.611s | real 14.943s |
| user 8.646s | user 8.693s |
| sys 5.779s | sys 5.464s |
|
CI had a failure that stopped further tests from running. We need to investigate to determine the root cause. SHA at time of execution: adf762e. Possible causes:
A blank commit, or merging to head, will be required to resume running CI for this PR. Error Details: Stack trace: |
Previously the license script looked for licenses within the engine/src/flutter tree. This PR updates the script to support the move of Abseil and the Fuchsia SDK from engine/src/flutter to engine/src/third_party. See flutter#177059 and flutter#177118
jason-simmons
force-pushed
the
license_crawl_root_src
branch
from
adf762e to
dbe7a7e
Compare
| std::string root_package_name = flags.root_package_name.has_value() | ||
| ? flags.root_package_name.value() | ||
| : GetDirFilename(working_dir); |
There was a problem hiding this comment.
nit: flags.root_package_name.value_or(GetDirFilename(working_dir))
There was a problem hiding this comment.
I wanted to avoid calling GetDirFilename in the common case where the root package name has a value. (The engine's current C++20 toolchain does not provide std::optional::or_else)
There was a problem hiding this comment.
I thought they were doing metaprogramming magic to only evaluate where there is no value. That sucks. or_else looks like more boilerplate than just using the ternary operator. TIL.
jason-simmons
added
the
autosubmit
flutter-dashboard
bot
removed
the
autosubmit
Roll Flutter from 75004a639ae4 to cb18290fa45e (48 revisions) flutter/flutter@75004a6...cb18290 2025-10-24 [email protected] Remove unnecessary `deprecated` withOpacity in `text_button.0.dart` in examples (flutter/flutter#177374) 2025-10-24 [email protected] Roll Packages from 9ec29b6 to 53d6138 (3 revisions) (flutter/flutter#177502) 2025-10-24 [email protected] Roll Dart SDK from d3248b00f545 to a0480f399f8f (1 revision) (flutter/flutter#177498) 2025-10-24 [email protected] Roll Skia from a47931d09585 to 3ed332b77bec (3 revisions) (flutter/flutter#177487) 2025-10-24 [email protected] Roll Abseil to Chromium's 5b92b04a2e (based on Abseil commit fc4481e968) (flutter/flutter#177059) 2025-10-24 98614782+auto-submit[bot]@users.noreply.github.com Reverts "Reverts "Enhance PR template with changelog and impact details (#177333)" (#177468)" (flutter/flutter#177499) 2025-10-24 [email protected] Document DropdownMenu showTrailingIcon and decorationBuilder interaction (flutter/flutter#177488) 2025-10-24 [email protected] Roll Dart SDK from 4a65fb890852 to d3248b00f545 (1 revision) (flutter/flutter#177486) 2025-10-24 [email protected] Fix bottom sheet Semantics route label for mismatched platforms (flutter/flutter#177094) 2025-10-24 [email protected] Roll Skia from eba11de00d5b to a47931d09585 (5 revisions) (flutter/flutter#177481) 2025-10-24 [email protected] Fix Dialog Semantics label and flags for mismatched platforms (flutter/flutter#176781) 2025-10-24 [email protected] Fix drawer Semantics for mismatched platforms (flutter/flutter#177095) 2025-10-24 [email protected] Roll Dart SDK from f7751ccea102 to 4a65fb890852 (2 revisions) (flutter/flutter#177480) 2025-10-23 [email protected] Change the root path of the license crawl to engine/src (flutter/flutter#177352) 2025-10-23 [email protected] [Material] Change default mouse cursor of buttons to basic arrow instead of click (except on web) (flutter/flutter#171796) 2025-10-23 [email protected] [Desktop] Propagate SemanticsNode::identifier to AXPlatformNodeDelegate::AuthorUniqueId (flutter/flutter#175405) 2025-10-23 [email protected] Roll Skia from 59ef57f4104e to eba11de00d5b (5 revisions) (flutter/flutter#177461) 2025-10-23 [email protected] Roll customer tests (flutter/flutter#177409) 2025-10-23 [email protected] Update CHANGELOG 3.35.7 notes (flutter/flutter#177463) 2025-10-23 [email protected] Marks Mac module_uiscene_test_ios to be unflaky (flutter/flutter#177378) 2025-10-23 [email protected] Allow empty dart defines in `flutter assemble` (flutter/flutter#177198) 2025-10-23 [email protected] Roll Packages from d113bbc to 9ec29b6 (12 revisions) (flutter/flutter#177455) 2025-10-23 [email protected] Implements engine-side declarative pointer event handling for semantics. (flutter/flutter#176974) 2025-10-23 [email protected] Fix the platform name of the windowing_test target for macOS in ci.yaml (flutter/flutter#177472) 2025-10-23 98614782+auto-submit[bot]@users.noreply.github.com Reverts "Enhance PR template with changelog and impact details (#177333)" (flutter/flutter#177468) 2025-10-23 [email protected] Change Flutter APIs to use spans (flutter/flutter#177272) 2025-10-23 [email protected] Roll Dart SDK from 020988602772 to f7751ccea102 (4 revisions) (flutter/flutter#177449) 2025-10-23 [email protected] [macOS] Implement regular window (flutter/flutter#176361) 2025-10-23 [email protected] Enhance PR template with changelog and impact details (flutter/flutter#177333) 2025-10-23 [email protected] Roll Skia from 6e6acf6644ef to 59ef57f4104e (2 revisions) (flutter/flutter#177436) 2025-10-23 [email protected] Add directional static members to AlignmentGeometry. (flutter/flutter#176571) 2025-10-23 [email protected] Roll ICU from 1b2e3e8a421e to ff35c4f9df23 (5 revisions) (flutter/flutter#177434) 2025-10-23 [email protected] Adds a new CI build for Linux host DDM-enabled artifacts (flutter/flutter#177252) 2025-10-23 [email protected] Roll Skia from 920cdcadd74c to 6e6acf6644ef (1 revision) (flutter/flutter#177432) 2025-10-23 [email protected] Roll Skia from 8cd449e4953b to 920cdcadd74c (6 revisions) (flutter/flutter#177426) 2025-10-23 [email protected] Added ahem license (flutter/flutter#177423) 2025-10-23 [email protected] Roll Dart SDK from 75f6ccb9bdc5 to 020988602772 (1 revision) (flutter/flutter#177421) 2025-10-23 [email protected] [web] Set `pointer-events: none` for img-element-backed images (flutter/flutter#177418) 2025-10-22 [email protected] Remove the x64 version of build_ios_framework_module_test (flutter/flutter#177136) 2025-10-22 [email protected] Fix accessibility events not being correctly translated to ATK (flutter/flutter#176991) 2025-10-22 [email protected] Roll Skia from b55bd60ed95b to 8cd449e4953b (8 revisions) (flutter/flutter#177405) 2025-10-22 [email protected] Roll Dart SDK from c23010c4f9e6 to 75f6ccb9bdc5 (4 revisions) (flutter/flutter#177399) 2025-10-22 [email protected] Fixes crash when adding and removing mulitple page-based route (flutter/flutter#177338) 2025-10-22 [email protected] Move child parameter to end of RefreshIndicator constructor (flutter/flutter#177019) 2025-10-22 [email protected] refactor: migrate OpenUpwardsPageTransitionsBuilder to widgets (flutter/flutter#177080) 2025-10-22 [email protected] Delete stray 'text' file (flutter/flutter#177355) ...
Previously the license script looked for licenses within the engine/src/flutter tree. This PR updates the script to support the move of Abseil and the Fuchsia SDK from engine/src/flutter to engine/src/third_party. See flutter#177059 and flutter#177118
2 participants
Previously the license script looked for licenses within the engine/src/flutter tree. This PR updates the script to support the move of Abseil and the Fuchsia SDK from engine/src/flutter to engine/src/third_party.
See #177059 and #177118