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

Use the 'dart compile js-dev' command for invoking DDC … #3768

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

a-siva
Copy link
Contributor

@a-siva a-siva commented Oct 30, 2024

Use the 'dart compile js-dev' command for invoking DDC instead of reaching into the dart-sdk and using the snapshot which is an internal implementation detail of the dart sdk.

…ching into the dart-sdk and using the snapshot which is an internal implementation detail of the dart sdk.
Copy link

github-actions bot commented Oct 30, 2024

PR Health

Changelog Entry ✔️
Package Changed Files

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

@kevmoo kevmoo requested a review from jakemac53 October 30, 2024 21:51
build_modules/pubspec.yaml Outdated Show resolved Hide resolved
build_modules/pubspec.yaml Outdated Show resolved Hide resolved
@jakemac53
Copy link
Contributor

Once we have a released dev SDK to depend on, I can update the CI jobs etc and push that here.

@jakemac53
Copy link
Contributor

cc @a-siva @nshahan something seems to be broken with the dart compile js-dev command on windows? I am not getting a lot of useful information from the GitHub logs though.

@a-siva
Copy link
Contributor Author

a-siva commented Oct 31, 2024

cc @a-siva @nshahan something seems to be broken with the dart compile js-dev command on windows? I am not getting a lot of useful information from the GitHub logs though.

Will investigate, looks like tests are waiting for temp directories to get deleted but they are in use, could be a test issue.

@jakemac53
Copy link
Contributor

Will investigate, looks like tests are waiting for temp directories to get deleted but they are in use, could be a test issue.

I think that possibly the worker isn't shutting down like it normally would.

@jakemac53
Copy link
Contributor

Note that we just send it a sigterm by calling Process.kill. I don't know how the dart tool deals with that, and if it can handle shutting down its sub-processes etc.

@a-siva
Copy link
Contributor Author

a-siva commented Nov 1, 2024

Note that we just send it a sigterm by calling Process.kill. I don't know how the dart tool deals with that, and if it can handle shutting down its sub-processes etc.

It does not handle shutting down sub-processes, if that is the issue we should also see errors on other platforms (linux, mac)

@jakemac53
Copy link
Contributor

It does not handle shutting down sub-processes, if that is the issue we should also see errors on other platforms (linux, mac)

Well, only windows won't allow deleting a directory because some sub-process has file handles in that directory still open. So it is possible that is the only reason we don't see the error on other platforms. If the process is left dangling though that is of course bad.

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