Skip to content

printenv skips environment variables with invalid UTF-8#9702

Closed
sylvestre wants to merge 1 commit into
uutils:mainfrom
sylvestre:printenv
Closed

printenv skips environment variables with invalid UTF-8#9702
sylvestre wants to merge 1 commit into
uutils:mainfrom
sylvestre:printenv

Conversation

@sylvestre

Copy link
Copy Markdown
Contributor

closes: #9701

@github-actions

Copy link
Copy Markdown

GNU testsuite comparison:

Congrats! The gnu test tests/tail/inotify-dir-recreate is now passing!

@sylvestre
sylvestre force-pushed the printenv branch 2 times, most recently from 7bc8beb to 8c5873a Compare December 18, 2025 21:47
@github-actions

Copy link
Copy Markdown

GNU testsuite comparison:

Congrats! The gnu test tests/misc/nohup is no longer failing!
Note: The gnu test tests/tail/inotify-dir-recreate is now being skipped but was previously passing.

@github-actions

Copy link
Copy Markdown

GNU testsuite comparison:

Congrats! The gnu test tests/tail/inotify-dir-recreate is now passing!

@github-actions

Copy link
Copy Markdown

GNU testsuite comparison:

Note: The gnu test tests/tail/inotify-dir-recreate is now being skipped but was previously passing.

Comment on lines +5 to +8
#[cfg(unix)]
use std::ffi::OsString;
#[cfg(unix)]
use std::os::unix::ffi::OsStringExt;

@cakebaker cakebaker Dec 19, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I would move the imports to the test function, otherwise there are "unused import" warnings on Android and the BSDs.

@ChrisDryden

Copy link
Copy Markdown
Collaborator

I was thinking that for most of the code we can use a helper function like this:

  fn write_os_str(out: &mut impl Write, s: &OsStr) -> io::Result<()> {
      #[cfg(unix)]
      {
          use std::os::unix::ffi::OsStrExt;
          out.write_all(s.as_bytes())
      }
      #[cfg(not(unix))]
      {
          // On Windows, OsString to_string_lossy is lossless
          out.write_all(s.to_string_lossy().as_bytes())
      }
  }

So everything can be kept as OsString, then in all of the functions they have the same code path without having the inline cfg stuff. Would be good to have most of our code move away from String by default through this too

@ChrisDryden

Copy link
Copy Markdown
Collaborator

Hey this helper function actually already exists in: write_all_os

fn write_all_os(&mut self, buf: &OsStr) -> io::Result<()> {

@ChrisDryden

Copy link
Copy Markdown
Collaborator

To make an example of how it could be used here I made a PR doing the similar thing for the env command: #9144

Makes the implementation way smaller compared to the inlining

@asder8215

Copy link
Copy Markdown
Contributor

I think you accidentally referenced the wc PR, think you meant this #9726?

@sylvestre sylvestre mentioned this pull request Dec 19, 2025
@sylvestre sylvestre closed this Dec 22, 2025
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.

printenv skips environment variables with invalid UTF-8

4 participants