Skip to content

Conversation

jakemac53
Copy link
Contributor

@jakemac53 jakemac53 commented Jul 29, 2025

Should resolve #2137.

Dropped the sanitizeExecutablePath entirely as it no longer seems to be necessary (SDK issue was closed long ago)

@jakemac53 jakemac53 requested a review from a team as a code owner July 29, 2025 18:04
@jakemac53
Copy link
Contributor Author

cc @DanTup

Copy link

github-actions bot commented Jul 29, 2025

PR Health

Breaking changes ✔️
Package Change Current Version New Version Needed Version Looking good?
process None 5.0.4 5.0.5 5.0.4 ✔️
Changelog Entry ✔️
Package Changed Files

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

Coverage ✔️
File Coverage
pkgs/process/lib/src/interface/common.dart 💚 100 %
pkgs/process/lib/src/interface/local_process_manager.dart 💚 40 % ⬆️ Infinity %

This check for test coverage is informational (issues shown here will not fail the PR).

API leaks ✔️

The following packages contain symbols visible in the public API, but not exported by the library. Export these symbols or remove them from your publicly visible API.

Package Leaked API symbols
License Headers ✔️
// Copyright (c) 2025, the Dart project authors. Please see the AUTHORS file
// for details. All rights reserved. Use of this source code is governed by a
// BSD-style license that can be found in the LICENSE file.
Files
no missing headers

All source files should start with a license header.

Unrelated files missing license headers
Files
pkgs/bazel_worker/benchmark/benchmark.dart
pkgs/bazel_worker/example/client.dart
pkgs/bazel_worker/example/worker.dart
pkgs/benchmark_harness/integration_test/perf_benchmark_test.dart
pkgs/boolean_selector/example/example.dart
pkgs/clock/lib/clock.dart
pkgs/clock/lib/src/clock.dart
pkgs/clock/lib/src/default.dart
pkgs/clock/lib/src/stopwatch.dart
pkgs/clock/lib/src/utils.dart
pkgs/clock/test/clock_test.dart
pkgs/clock/test/default_test.dart
pkgs/clock/test/stopwatch_test.dart
pkgs/clock/test/utils.dart
pkgs/coverage/lib/src/coverage_options.dart
pkgs/html/example/main.dart
pkgs/html/lib/dom.dart
pkgs/html/lib/dom_parsing.dart
pkgs/html/lib/html_escape.dart
pkgs/html/lib/parser.dart
pkgs/html/lib/src/constants.dart
pkgs/html/lib/src/encoding_parser.dart
pkgs/html/lib/src/html_input_stream.dart
pkgs/html/lib/src/list_proxy.dart
pkgs/html/lib/src/query_selector.dart
pkgs/html/lib/src/token.dart
pkgs/html/lib/src/tokenizer.dart
pkgs/html/lib/src/treebuilder.dart
pkgs/html/lib/src/utils.dart
pkgs/html/test/dom_test.dart
pkgs/html/test/parser_feature_test.dart
pkgs/html/test/parser_test.dart
pkgs/html/test/query_selector_test.dart
pkgs/html/test/selectors/level1_baseline_test.dart
pkgs/html/test/selectors/level1_lib.dart
pkgs/html/test/selectors/selectors.dart
pkgs/html/test/support.dart
pkgs/html/test/tokenizer_test.dart
pkgs/html/test/trie_test.dart
pkgs/html/tool/generate_trie.dart
pkgs/pubspec_parse/test/git_uri_test.dart
pkgs/stack_trace/example/example.dart
pkgs/watcher/test/custom_watcher_factory_test.dart
pkgs/yaml_edit/example/example.dart

Copy link

github-actions bot commented Jul 29, 2025

Package publishing

Package Version Status Publish tag (post-merge)
package:bazel_worker 1.1.3 already published at pub.dev
package:benchmark_harness 2.4.0-wip WIP (no publish necessary)
package:boolean_selector 2.1.2 already published at pub.dev
package:browser_launcher 1.1.3 already published at pub.dev
package:cli_config 0.2.1-wip WIP (no publish necessary)
package:cli_util 0.5.0-wip WIP (no publish necessary)
package:clock 1.1.3-wip WIP (no publish necessary)
package:code_builder 4.10.2-wip WIP (no publish necessary)
package:coverage 1.15.0 already published at pub.dev
package:csslib 1.0.2 already published at pub.dev
package:extension_discovery 2.1.0 already published at pub.dev
package:file 7.0.2-wip WIP (no publish necessary)
package:file_testing 3.1.0-wip WIP (no publish necessary)
package:glob 2.1.3 already published at pub.dev
package:graphs 2.3.3-wip WIP (no publish necessary)
package:html 0.15.6 already published at pub.dev
package:io 1.1.0-wip WIP (no publish necessary)
package:json_rpc_2 4.0.0 already published at pub.dev
package:markdown 7.3.1-wip WIP (no publish necessary)
package:mime 2.0.0 already published at pub.dev
package:oauth2 2.0.4-wip WIP (no publish necessary)
package:package_config 2.3.0-wip WIP (no publish necessary)
package:pool 1.5.2-wip WIP (no publish necessary)
package:process 5.0.5 ready to publish process-v5.0.5
package:pub_semver 2.2.0 already published at pub.dev
package:pubspec_parse 1.5.0 already published at pub.dev
package:source_map_stack_trace 2.1.3-wip WIP (no publish necessary)
package:source_maps 0.10.14-wip WIP (no publish necessary)
package:source_span 1.10.1 already published at pub.dev
package:sse 4.1.8 already published at pub.dev
package:stack_trace 1.12.1 already published at pub.dev
package:stream_channel 2.1.4 already published at pub.dev
package:stream_transform 2.1.2-wip WIP (no publish necessary)
package:string_scanner 1.4.1 already published at pub.dev
package:term_glyph 1.2.3-wip WIP (no publish necessary)
package:test_reflective_loader 0.3.0 already published at pub.dev
package:timing 1.0.2 already published at pub.dev
package:unified_analytics 8.0.5 already published at pub.dev
package:watcher 1.1.2 already published at pub.dev
package:yaml 3.1.3 already published at pub.dev
package:yaml_edit 2.2.2 already published at pub.dev

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

@jakemac53
Copy link
Contributor Author

Weird, I cannot replicate the originally reported error without this patch - the integration test I added at the end works either way.

@jakemac53
Copy link
Contributor Author

Ahhh, so I can replicate this error with runInShell: true.

@github-actions github-actions bot added the type-infra A repository infrastructure change or enhancement label Jul 29, 2025
@DanTup
Copy link
Contributor

DanTup commented Jul 29, 2025

@jakemac53 for my original issue, the folder was named "Latest (Bleeding Edge)" which contains both spaces and quotes, so I would've expected it to already be quoted (in fact in the screenshot, it looks wrapped in quotes). Might there be more to it?

I can do some testing locally tomorrow and see if I can confirm if this fixes it (and if not, try to figure out why).

@DanTup
Copy link
Contributor

DanTup commented Jul 29, 2025

Oh, this sounded familiar and I found this in Dart-Code:

	// Spawning processes on Windows with funny symbols in the path requires quoting. However if you quote an
	// executable with a space in its path and an argument also has a space, you have to then quote _all_ of the
	// arguments!
	// https://github.com/nodejs/node/issues/7367

https://github.com/Dart-Code/Dart-Code/blob/7f51d4c35f48b0bb69f7913aaa94e6b501e403f4/src/shared/processes.ts#L30-L33

Shell execute is a mess. If you know you're getting dart.exe (and not the dart shell script from Flutter's bin folder), I wonder if you should skip shell execute instead? In Dart-Code we now only use the shell if the target ends with .bat:

https://github.com/Dart-Code/Dart-Code/blob/7f51d4c35f48b0bb69f7913aaa94e6b501e403f4/src/shared/processes.ts#L19

@jakemac53
Copy link
Contributor Author

When testing locally on windows it seems like the dart issue has since been closed and now this helper code is actually causing problems when parentheses are involved.

I have now removed the helper and its tests, and added integration tests that actually run an executable, and also added windows testing to the GitHub workflows for this package.

@jakemac53 jakemac53 changed the title support and test parens on windows Fix mixtures of parentheses and spaces in windows command paths Jul 29, 2025
@jakemac53
Copy link
Contributor Author

Ok, so actually it looks like when there are only parenthesis this still doesn't work. However, simply wrapping it in quotes also doesn't work.

I don't know what the right thing to do here is.

@jakemac53
Copy link
Contributor Author

Shell execute is a mess. If you know you're getting dart.exe (and not the dart shell script from Flutter's bin folder), I wonder if you should skip shell execute instead? In Dart-Code we now only use the shell if the target ends with .bat:

I can try this, but dang does that sound hacky lol.

@jakemac53 jakemac53 added type-bug Incorrect behavior (everything from a crash to more subtle misbehavior) and removed type-infra A repository infrastructure change or enhancement labels Jul 29, 2025
@DanTup
Copy link
Contributor

DanTup commented Jul 30, 2025

Shell execute is a mess. If you know you're getting dart.exe (and not the dart shell script from Flutter's bin folder), I wonder if you should skip shell execute instead? In Dart-Code we now only use the shell if the target ends with .bat:

I can try this, but dang does that sound hacky lol.

Perhaps, but escape correctly for shell is already a minefield (see dart-lang/sdk#59604 for some issues that were never resolved). If you know you're executing a binary and don't need the shell, avoiding it makes things simpler.

I'll see if I can pull these changes in locally and test my original problem and post back.

Copy link
Contributor

@DanTup DanTup left a comment

Choose a reason for hiding this comment

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

I pulled these changes locally and tested with my original issue, and the tests ran successfully (where my SDK has both spaces and parens, and my test project has spaces):

image

@jakemac53
Copy link
Contributor Author

jakemac53 commented Jul 30, 2025

I restarted the windows jobs to see if they get picked up... if not I guess I will remove them as they don't ever seem to run 😭

Looks like it was just a bad OS name and GitHub likes to waste time queueing jobs for oses that don't exist :P

@github-actions github-actions bot added the type-infra A repository infrastructure change or enhancement label Jul 30, 2025
@jakemac53
Copy link
Contributor Author

jakemac53 commented Jul 30, 2025

Btw I skipped the test for having just parenthesis in the path and filed a separate issue. This is not a regression, its a new test that is failing (there was no special handling for this case before).

@jakemac53 jakemac53 merged commit 5e977d6 into main Jul 30, 2025
15 checks passed
@jakemac53 jakemac53 deleted the support-parens-windows branch July 30, 2025 14:40
jakemac53 added a commit to dart-lang/ai that referenced this pull request Jul 30, 2025
copybara-service bot pushed a commit to dart-lang/sdk that referenced this pull request Aug 4, 2025
…, web, webdriver

Revisions updated by `dart tools/rev_sdk_deps.dart`.

ai (https://github.com/dart-lang/ai/compare/7fbe88a..72a9283):
  72a9283  2025-07-31  Jacob MacDonald  add --exclude-tool option to exclude tools by name (dart-lang/ai#253)
  0dbbd8b  2025-07-30  Jacob MacDonald  only run in shell when on windows and running a .bat file (dart-lang/ai#252)
  0858b0b  2025-07-28  Jacob MacDonald  further prompt refinement, discourage use of temp ids and encourage getting the widget tree often (dart-lang/ai#249)
  912c1f3  2025-07-28  Jacob MacDonald  release dart_mcp v0.3.3 (dart-lang/ai#247)
  0f7cba8  2025-07-28  Jacob MacDonald  add analytics tracking for prompts (dart-lang/ai#246)

dartdoc (https://github.com/dart-lang/dartdoc/compare/882aea9..414953e):
  414953ed  2025-08-04  Sam Rawlins  Simplify how inheritance is computed. (dart-lang/dartdoc#4079)

ecosystem (https://github.com/dart-lang/ecosystem/compare/d5233c6..2fe3618):
  2fe3618  2025-08-01  dependabot[bot]  Bump the github-actions group with 3 updates (dart-lang/ecosystem#361)

protobuf (https://github.com/dart-lang/protobuf/compare/44ecd74..0b73b0d):
  0b73b0d  2025-07-30  Devon Carew  workspace, formatting, and proto version updates (google/protobuf.dart#1033)
  a664760  2025-07-30  Devon Carew  various CI workflow updates (google/protobuf.dart#1035)

test (https://github.com/dart-lang/test/compare/6aeb1e4..953e828):
  953e8282  2025-08-01  Nate Bosch  Drop executable arguments forwarding (dart-lang/test#2528)

tools (https://github.com/dart-lang/tools/compare/2a2a2d6..5e977d6):
  5e977d6f  2025-07-30  Jacob MacDonald  Fix mixtures of parentheses and spaces in windows command paths (dart-lang/tools#2138)
  607340ca  2025-07-29  Devon Carew  disable failing test (dart-lang/tools#2136)

vector_math (https://github.com/google/vector_math.dart/compare/13f185f..3939545):
  3939545  2025-08-04  Kevin Moore  Bump min SDK to 3.7, update dependencies, reformat (google/vector_math.dart#348)

web (https://github.com/dart-lang/web/compare/da1dd5d..1d5771b):
  1d5771b  2025-07-31  Nikechukwu  [interop] Add support for importing and exporting declarations, as well as multi-file output (dart-lang/web#418)

webdriver (https://github.com/google/webdriver.dart/compare/cfab787..595649d):
  595649d  2025-08-01  dependabot[bot]  Bump nanasess/setup-chromedriver (google/webdriver.dart#331)

Change-Id: Ia014bf6cafa9edcdaf453edda9cd5ecff516e16d
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/443625
Auto-Submit: Devon Carew <[email protected]>
Reviewed-by: Konstantin Shcheglov <[email protected]>
Commit-Queue: Konstantin Shcheglov <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

package:process type-bug Incorrect behavior (everything from a crash to more subtle misbehavior) type-infra A repository infrastructure change or enhancement

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Unable to run tests via MCP server tool (on Windows?)

3 participants