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.

Sign up for GitHub

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

Jump to bottom

Send ScalaNativeWorker logging to stderr #4418

Merged
merged 3 commits into from
Jan 28, 2025
Merged

Send ScalaNativeWorker logging to stderr #4418

merged 3 commits into from
Jan 28, 2025

Conversation

btrachey
Copy link
Contributor

@btrachey btrachey commented Jan 28, 2025 •
edited
Loading

Some log levels for the ScalaNativeWorker were writing to stdout instead of stderr, this switches those over to stderr.
Closes #4409

@btrachey btrachey force-pushed the native-module-stderr-logging branch from 155af7d to fa904fe Compare January 28, 2025 03:49
@lefou
Copy link
Member

lefou commented Jan 28, 2025 •
edited
Loading

I'd assume this change will not fix #4409, since show is already redirecting the out channel to err for all other tasks. If it fixes this nevertheless, than the real issue might be the handling of the stream in the relevant ScalaNativeWorker. I haven't looked at it though.

@btrachey Have you tested this change and found it fixing the issue?

@lolgab
Copy link
Member

lolgab commented Jan 28, 2025

@lefou Regardless of fixing the issue, I think it's a good change to merge, since we always System.err in Scala.js worker.

lefou
lefou approved these changes Jan 28, 2025
View reviewed changes
Copy link
Member

@lefou lefou left a comment

Choose a reason for hiding this comment

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

Looks good to me. Thank you for this PR, @btrachey.

@lefou lefou marked this pull request as ready for review January 28, 2025 12:02
@btrachey
Copy link
Contributor Author

Yes, this change did fix the problem I was having; I built mill from my fork and tested without and with this change.

@lihaoyi lihaoyi merged commit 714c42b into com-lihaoyi:main Jan 28, 2025
30 of 31 checks passed
@lefou lefou added this to the 0.12.6 milestone Jan 28, 2025
@btrachey btrachey deleted the native-module-stderr-logging branch January 29, 2025 02:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lolgab lolgab lolgab approved these changes

@lefou lefou lefou approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
0.12.6
Development

Successfully merging this pull request may close these issues.

Tasks from ScalaNative module logging to stdout instead of stderr
4 participants
@btrachey @lefou @lolgab @lihaoyi