Write to stdout if - is given as output file

[pjhades]

With this PR, if -o - or --emit KIND=- is provided, output will be written to stdout instead. Binary output (those of type obj, llvm-bc, link and metadata) being written this way will result in an error unless stdout is not a tty. Multiple output types going to stdout will trigger an error too, as they will all be mixded together.

This implements rust-lang/compiler-team#431

The idea behind the changes is to introduce an OutFileName enum that represents the output - be it a real path or stdout - and to use this enum along the code paths that handle different output types.

[rustbot]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @b-naber (or someone else) soon.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

• @rustbot author: the review is finished, PR author should check the comments and take action accordingly
• @rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

[rustbot]

These commits modify the Cargo.lock file. Unintentional changes to Cargo.lock can be introduced when switching branches and rebasing PRs.

If this was unintentional then you should revert the changes before this PR is merged.
Otherwise, you can ignore this comment.

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

[rustbot]

These commits modify the Cargo.lock file. Unintentional changes to Cargo.lock can be introduced when switching branches and rebasing PRs.

If this was unintentional then you should revert the changes before this PR is merged.
Otherwise, you can ignore this comment.

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

[bors]

☔ The latest upstream changes (presumably #111641) made this pull request unmergeable. Please resolve the merge conflicts.

[bors]

☔ The latest upstream changes (presumably #111858) made this pull request unmergeable. Please resolve the merge conflicts.

[b-naber]

Sorry for the late review. This looks good to me, but some nits.

[b-naber]

This could be included in a OutputFileName::New function.

[pjhades]

Do you mean defining something like this?

impl OutFileName {
    pub fn temp_path(&self, flavor: OutputType, codegen_unit_name: Option<&str>) -> PathBuf {
      match *self {
        OutFileName::Real(ref path) => path.clone(),
        OutFileName::Stdout => outputs.temp_path(flavor, codegen_unit_name),
      }
    }
}

Then use it here:

let out_filename = output.temp_path();

[b-naber]

I read this wrong initially. But I think something like this should work (didn't compile this):

impl OutFileName {
  pub fn process<'a>(&self, sess: &'a Session, tempfiles_for_stdout_output: &mut Vec<PathBuf>, outputs: &OutputFilenames) {
    match self {
      Self::Real(_) => {/* Don't think we need to do anything here */ }
      Self::Stdout => {
        let crate_name = format!("{}", codegen_results.crate_info.local_crate_name);
        let temp_filename =
            outputs.temp_path(OutputType::Exe, Some(crate_name.as_str()));          

        if self.is_tty() {
            sess.emit_err(errors::BinaryOutputToTty { 
                shorthand: OutputType::Exe.shorthand(),
            });
        } else if let Err(e) = copy_to_stdout(&temp_filename) {
            sess.emit_err(errors::CopyPath::new(&temp_filename, self.as_path(), e));
        } 

        tempfiles_for_stdout_output.push(temp_filename);
       
      }
    }
  }
}

and then use out_filename(..).process(...)

Which seems better to me than including all of this logic in link_binary.

[pjhades]

Which seems better to me than including all of this logic in link_binary.

Yeah, that's a fair point. I thought about this.

Here basically I wanted to do two things, before and after linking:

• Before linking, generate a filename for the file that would hold the result of linking
• Link as usual
• After linking, check if we need to copy the content of that file to stdout

It's a bit hard to put the two things in one function. I think it makes more sense to just abstract the before-linking part, like I commented previously. Regarding the after-linking part, it doesn't feel natural to me if I put it in a function like the following, because it feels like the self here is just a thin layer for the convenience of calling a function, and the function does a bunch of things somewhat unrelated to self:

impl OutFileName {
    pub fn copy_file_to_stdout<'a>(&self, flavor: OutputType, file: &PathBuf, sess: &'a Session) {
        match *self {
            OutFileName::Real(_) => ();
            OutFileName::Stdout => {
                if self.is_tty() {
                    sess.emit_err(errors::BinaryOutputToTty {
                        shorthand: flavor.shorthand(),
                    });
                } else if let Err(e) = copy_to_stdout(file) {
                    sess.emit_err(errors::CopyPath::new(file, self.as_path(), e));
                }
            }
        }
    }
}

What do you think?

[b-naber]

Seems good. For the part before linking you could also have a method like get_filename on OutFileName, can't you?

[pjhades]

Yes I can. I added two helpers to OutFileName and simplified the part before linking. IMHO, I'm leaning towards keeping the after-linking part as is, because - in addition to my point above - the logic is one of the steps of link_binary when the output is stdout: the content of the linked binary should be written to the desired destination.

[b-naber]

LGTM. Thanks for the PR.

@bors r+

[bors]

📌 Commit 6622590ac0a32cc477365237885dd7a129fbd908 has been approved by b-naber

It is now in the queue for this repository.

[bors]

⌛ Testing commit 6622590ac0a32cc477365237885dd7a129fbd908 with merge f6e351f8a4a34f7a83fd7e590dda9a895442e781...

[bors]

💔 Test failed - checks-actions

[pjhades]

@rustbot review

I fixed the original test and also another bug that would show up in x86-msvc jobs.

[b-naber]

Can you explain what exactly you did? What other bug do you think you have found?

Also can you please not squash all new changes into old commits? This makes it really tedious to review, because the reviewer needs to go through all changes again.

[pjhades]

@b-naber My bad, I restored the history. Hopefully it should be easier to read.

For the original error

arm-none-eabi-gcc: error: unrecognized command line option '-m64'

It was because my test program tests/run-make/emit-to-stdout/test.rs was linked as a binary crate. So to fix this I specified the crate type as rlib, in the same way as tests/run-make/emit-named-files/foo.rs, as these two tests have a similar structure.

After the fix build passed, but CI job dist-various-2 reported error

cp: cannot stat 'obj/build/metrics.json': No such file or directory

This was because the config option build.metrics was not set in the CI script in this case. If I understand correctly, build.metrics will be set if the CI job is triggered by bors. (The check isCiBranch auto will pass.) So I don't think I need to do anything regarding this error.

Finally I ran CI job x86_64-msvc-1, which complained:

error: failed to remove C:\a\rust\rust\build\x86_64-pc-windows-msvc\test\run-make\emit-to-stdout\emit-to-stdout\out\rmetaMyDjZM\lib.rmeta: Access is denied. (os error 5)

This happened because I tried to remove the temporary metadata file but I didn't have the permission in the test environment somehow. But actually I don't have to manually delete the temporary metadata file because it is managed by MaybeTempDir, which will be automatically deleted by the type's drop(). So I removed the file-deleting logic and x86_64-msvc-1 passed.

[b-naber]

@bors r+ rollup=iffy

[bors]

📌 Commit ade6c36 has been approved by b-naber

It is now in the queue for this repository.

[bors]

⌛ Testing commit ade6c36 with merge 343ad6f...

[bors]

☀️ Test successful - checks-actions
Approved by: b-naber
Pushing 343ad6f to master...

[rust-timer]

Finished benchmarking commit (343ad6f): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-2.1%	[-2.1%, -2.1%]	1
All ❌✅ (primary)	-	-	0

Cycles

This benchmark run did not return any relevant results for this metric.

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 648.999s -> 647.29s (-0.26%)