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

Add support for multiple full paths on macos #2276

Merged
merged 22 commits into from
Sep 13, 2024
Merged

Conversation

natebosch
Copy link
Member

@natebosch natebosch commented Sep 3, 2024

We cannot look up multiple basename commands in the system path and the
current macOsExecutable configuration may have existing uses in
dart_test.yaml files so it isn't safe to require full paths. Add a
separate macOsAbsolutePaths configuration to enable internal
definitions that check multiple full paths and execute the first one
that exists.

Add the basename firefox as a fallback. Adding that command to the
path is one workaround for users with firefox installed in an unexpected
location. This is also the only approach that is compatible with mac on
GitHub actions using the setup-firefox action.

There is no support here for dart_test.yaml. Users that are
configuring an executable for macOS will continue to have support for
only a single value, even if they are specifying an absolute path.

Tests remain skipped because our mono repo setup does not have a way to
include the setup-firefox action.

We cannot look up multiple basename commands in the system path and the
current `macOsExecutable` configuration may have existing uses in
`dart_test.yaml` files so it isn't safe to require full paths. Add a
separate `macOsAbsolutePaths` configuration to enable internal
definitions that check multiple full paths and execute the first one
that exists.

There is no support here for `dart_test.yaml`. Users that are
configuring an executable for macOS will continue to have support for
only a single value, even if they are specifying an absolute path.
Copy link

github-actions bot commented Sep 3, 2024

PR Health

Changelog Entry ✔️
Package Changed Files

Changes to files need to be accounted for in their respective changelogs.

Package publish validation ✔️
Package Version Status
package:checks 0.3.1-wip WIP (no publish necessary)
package:test 1.25.9-wip WIP (no publish necessary)
package:test_api 0.7.4-wip WIP (no publish necessary)
package:test_core 0.6.6-wip WIP (no publish necessary)

Documentation at https://github.com/dart-lang/ecosystem/wiki/Publishing-automation.

@natebosch
Copy link
Member Author

This is the least invasive way I could see to make this change. I'm not sure if it's worth landing, but opening the PR for discussion.

@natebosch
Copy link
Member Author

I think if we wanted we could support this in dart_test.yaml. We could accept either a single string, or a list of strings, and we could validate when parsing that only full paths are used if it is a list. We already have some inconsistency between the meaning of the arguments across OSes, so I don't think it's a particular problem to add this option/caveat exclusive to macOS.

fixes merge behavior to correctly override.
pkgs/test/lib/src/runner/executable_settings.dart Outdated Show resolved Hide resolved
pkgs/test/lib/src/runner/executable_settings.dart Outdated Show resolved Hide resolved
natebosch and others added 2 commits September 5, 2024 13:34
Co-authored-by: Jacob MacDonald <jakemac@google.com>
Co-authored-by: Jacob MacDonald <jakemac@google.com>
@natebosch natebosch marked this pull request as ready for review September 10, 2024 23:06
@natebosch natebosch requested a review from a team as a code owner September 10, 2024 23:06
natebosch added a commit that referenced this pull request Sep 11, 2024
This will exercise things like launching browsers (where they are
currently working on CI).

Add `osx` to the sharded `package:test` tests alongside `windows`. These
exercise the majority of OS specific behavior.

Skip existing safari tests due to a bug running tests in non-interactive
environments. (#1253)

Skip existing firefox tests due to a changed executable path. (Planned
fix in #2276)

Refactor a ternary chain to an if/else chain to add the extra
conditions for firefox and safari.
@natebosch
Copy link
Member Author

This fixes the problem for me on a local mac. It looks like maybe firefox doesn't come by default on github actions macos runs? I don't think mono package supports adding a firefox setup step. Any suggestions on a workaround @kevmoo ?

@natebosch
Copy link
Member Author

It looks like maybe firefox doesn't come by default on github actions macos runs?

No, it still fails even if I manually add the setup-firefox steps.

@natebosch
Copy link
Member Author

I cannot figure out the mac problem. Landing this is still worth it because it does fix things for me locally. @jakemac53 any concerns about landing this without unskipping the tests on CI, or with using the basename as a fallback?

@jakemac53
Copy link
Contributor

I cannot figure out the mac problem. Landing this is still worth it because it does fix things for me locally. @jakemac53 any concerns about landing this without unskipping the tests on CI, or with using the basename as a fallback?

I am fine with it, we don't have a lot of people asking about this, so it isn't worth dumping a ton of time into. If people do get broken again they will file issues (plus, there is a workaround to set your own path).

@natebosch natebosch merged commit 22835e2 into master Sep 13, 2024
56 checks passed
@natebosch natebosch deleted the macos-full-paths branch September 13, 2024 18:13
copybara-service bot pushed a commit to dart-lang/sdk that referenced this pull request Sep 23, 2024
Revisions updated by `dart tools/rev_sdk_deps.dart`.

dartdoc (https://github.com/dart-lang/dartdoc/compare/8100ccf..526dbd5):
  526dbd55  2024-09-15  Sam Rawlins  Strip the enclosing package's lower-case name from a library's dir name (dart-lang/dartdoc#3883)
  98dd9dad  2024-09-13  Sam Rawlins  Add output-size info to the '--stats' flag on various tasks. (dart-lang/dartdoc#3879)
  090b05ba  2024-09-13  Sam Rawlins  Bump to 8.2.0 (dart-lang/dartdoc#3880)
  0a529762  2024-09-12  Sam Rawlins  Fix search which is not using canonical libraries as enclosing elements (dart-lang/dartdoc#3877)
  d4dff9a4  2024-09-12  Konstantin Scheglov  Stop using 'augmented' from the element model. (dart-lang/dartdoc#3878)
  d6b865f9  2024-09-11  Sam Rawlins  Support multiple partial calls from a single template (dart-lang/dartdoc#3873)
  25c9edf3  2024-09-10  Parker Lougheed  Standardize to newer SDK APIs (dart-lang/dartdoc#3872)
  b61de2f2  2024-09-10  Sam Rawlins  Clean up some sorting, naming, and doc nits (dart-lang/dartdoc#3876)
  55ce1164  2024-09-10  Sam Rawlins  Use latest language version for DartFormatter (dart-lang/dartdoc#3875)
  2a262818  2024-09-10  Sam Rawlins  Comply with analyzer 6.9.0 APIs (dart-lang/dartdoc#3874)
  ca98b898  2024-09-09  Sam Rawlins  Include extension members on the extended type's page (dart-lang/dartdoc#3863)

mockito (https://github.com/dart-lang/mockito/compare/d0fda0c..b66be81):
  b66be81  2024-09-13  Googler  Migration for analyzer APIs.

stack_trace (https://github.com/dart-lang/stack_trace/compare/090d3d1..5b82965):
  5b82965  2024-09-19  Ömer Sinan Ağacan  Relax URI matching in V8 Wasm frame regex (dart-lang/stack_trace#161)
  9b1ed4f  2024-09-19  Ömer Sinan Ağacan  Add support for parsing Wasm stack frames of Chrome (V8), Firefox, Safari (dart-lang/stack_trace#159)
  d38eee8  2024-09-17  Ömer Sinan Ağacan  Fix analysis issues (dart-lang/stack_trace#160)

test (https://github.com/dart-lang/test/compare/9a2d155..22835e2):
  22835e2e  2024-09-13  Nate Bosch  Add support for multiple full paths on macos (dart-lang/test#2276)

web (https://github.com/dart-lang/web/compare/933a37d..d8549a3):
  d8549a3  2024-09-13  Srujan Gaddam   Add a generate-all flag to emit all bindings (dart-lang/web#302)

Change-Id: Ib07e4c0eda9c8ea180ef989c7d9cf20c390457e8
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/386260
Commit-Queue: Konstantin Shcheglov <scheglov@google.com>
Auto-Submit: Devon Carew <devoncarew@google.com>
Reviewed-by: Konstantin Shcheglov <scheglov@google.com>
FMorschel pushed a commit to FMorschel/sdk that referenced this pull request Sep 25, 2024
Revisions updated by `dart tools/rev_sdk_deps.dart`.

dartdoc (https://github.com/dart-lang/dartdoc/compare/8100ccf..526dbd5):
  526dbd55  2024-09-15  Sam Rawlins  Strip the enclosing package's lower-case name from a library's dir name (dart-lang/dartdoc#3883)
  98dd9dad  2024-09-13  Sam Rawlins  Add output-size info to the '--stats' flag on various tasks. (dart-lang/dartdoc#3879)
  090b05ba  2024-09-13  Sam Rawlins  Bump to 8.2.0 (dart-lang/dartdoc#3880)
  0a529762  2024-09-12  Sam Rawlins  Fix search which is not using canonical libraries as enclosing elements (dart-lang/dartdoc#3877)
  d4dff9a4  2024-09-12  Konstantin Scheglov  Stop using 'augmented' from the element model. (dart-lang/dartdoc#3878)
  d6b865f9  2024-09-11  Sam Rawlins  Support multiple partial calls from a single template (dart-lang/dartdoc#3873)
  25c9edf3  2024-09-10  Parker Lougheed  Standardize to newer SDK APIs (dart-lang/dartdoc#3872)
  b61de2f2  2024-09-10  Sam Rawlins  Clean up some sorting, naming, and doc nits (dart-lang/dartdoc#3876)
  55ce1164  2024-09-10  Sam Rawlins  Use latest language version for DartFormatter (dart-lang/dartdoc#3875)
  2a262818  2024-09-10  Sam Rawlins  Comply with analyzer 6.9.0 APIs (dart-lang/dartdoc#3874)
  ca98b898  2024-09-09  Sam Rawlins  Include extension members on the extended type's page (dart-lang/dartdoc#3863)

mockito (https://github.com/dart-lang/mockito/compare/d0fda0c..b66be81):
  b66be81  2024-09-13  Googler  Migration for analyzer APIs.

stack_trace (https://github.com/dart-lang/stack_trace/compare/090d3d1..5b82965):
  5b82965  2024-09-19  Ömer Sinan Ağacan  Relax URI matching in V8 Wasm frame regex (dart-lang/stack_trace#161)
  9b1ed4f  2024-09-19  Ömer Sinan Ağacan  Add support for parsing Wasm stack frames of Chrome (V8), Firefox, Safari (dart-lang/stack_trace#159)
  d38eee8  2024-09-17  Ömer Sinan Ağacan  Fix analysis issues (dart-lang/stack_trace#160)

test (https://github.com/dart-lang/test/compare/9a2d155..22835e2):
  22835e2e  2024-09-13  Nate Bosch  Add support for multiple full paths on macos (dart-lang/test#2276)

web (https://github.com/dart-lang/web/compare/933a37d..d8549a3):
  d8549a3  2024-09-13  Srujan Gaddam   Add a generate-all flag to emit all bindings (dart-lang/web#302)

Change-Id: Ib07e4c0eda9c8ea180ef989c7d9cf20c390457e8
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/386260
Commit-Queue: Konstantin Shcheglov <scheglov@google.com>
Auto-Submit: Devon Carew <devoncarew@google.com>
Reviewed-by: Konstantin Shcheglov <scheglov@google.com>
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.

2 participants