Write to stdout if `-` is given as output file #111626

Merged

Conversation

Contributor

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.

Collaborator

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

added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

labels

May 16, 2023

Collaborator

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

This comment has been minimized.

Collaborator

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

This comment has been minimized.

Contributor

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

Contributor

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

Contributor

b-naber

left a comment

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

compiler/rustc_codegen_ssa/messages.ftl

Outdated Show resolved

compiler/rustc_codegen_ssa/src/back/link.rs

Outdated

		sess,
		crate_type,
		outputs,
		codegen_results.crate_info.local_crate_name,
		);
		let out_filename = match output {

Contributor

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

Contributor

Author

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();

Contributor

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.

Contributor

Author

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?

Contributor

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

Contributor

Author

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.

compiler/rustc_codegen_ssa/src/back/link.rs

Outdated Show resolved

compiler/rustc_driver_impl/src/lib.rs

Show resolved

compiler/rustc_interface/src/passes.rs

Outdated Show resolved

compiler/rustc_interface/src/passes.rs

Show resolved

compiler/rustc_interface/src/util.rs

Outdated Show resolved

compiler/rustc_mir_transform/src/dump_mir.rs

Show resolved

src/doc/rustc/src/command-line-arguments.md

Outdated Show resolved

Contributor

LGTM. Thanks for the PR.

@bors r+

pjhades reacted with heart emoji

Contributor

Commit 6622590 has been approved by b-naber

It is now in the queue for this repository.

bors

added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.

and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.

labels

Jun 2, 2023

Contributor

Testing commit 6622590 with merge f6e351f...

This comment has been minimized.

Contributor

Test failed - checks-actions

bors

added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.

and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.

labels

Jun 2, 2023

This comment has been minimized.

Contributor

Sorry that I can't help you here, I'm not really familiar with that test suite. Have your tried asking for help on zulip?

Contributor

Author

@b-naber Thanks for checking back. I haven't asked on Zulip, but I found that the -m64 argument issue was because my test program was compiled as a binary crate, so -m64 option was passed to the linker but the linker didn't support the option. (I'm not sure why -m64 is passed for binary crates but not for libraries.) I've changed the crate type to rlib and the issue went away.

There's another issue of missing metrics.json. In src/ci/run.sh there's an if-branch:

if ! isCI || isCiBranch auto || isCiBranch beta || isCiBranch try || isCiBranch try-perf; then
    RUST_CONFIGURE_ARGS="$RUST_CONFIGURE_ARGS --set build.print-step-timings --enable-verbose-tests"
    RUST_CONFIGURE_ARGS="$RUST_CONFIGURE_ARGS --set build.metrics"
    HAS_METRICS=1
fi

The check would fail if I run for example the dist-various-1 test job myself in the CI. But for the build that you triggered by r+ the branch being built would be refs/heads/auto so the if-branch would execute and generate the metrics.

I'll run a windows build to see if any error happens. If it still complains about missing metrics.json I'll ping you to run the tests via r+.

This comment has been minimized.

Contributor

Author

@rustbot review

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

rustbot

added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.

and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author.

labels

Jun 6, 2023

Contributor

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.

Contributor

Author

@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.

Contributor

@bors r+ rollup=iffy

Contributor

Commit ade6c36 has been approved by b-naber

It is now in the queue for this repository.

bors

added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.

and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.

labels

Jun 9, 2023

Contributor

Testing commit ade6c36 with merge 343ad6f...

Contributor

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

bors

added the merged-by-bors This PR was explicitly merged by bors label

Jun 9, 2023

bors

merged commit 343ad6f into

rust-lang:master

Jun 9, 2023

12 checks passed

rustbot

added this to the 1.72.0 milestone

Jun 9, 2023

Collaborator

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

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%)

Reviewers

b-naber

b-naber left review comments

Assignees

b-naber

Labels

A-testsuite Area: The testsuite used to check the correctness of rustc merged-by-bors This PR was explicitly merged by bors S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue.

Projects

None yet

Milestone

1.72.0

Development

Successfully merging this pull request may close these issues.

None yet

6 participants

Write to stdout if `-` is given as output file by pjhades · Pull Request #111626...

Write to stdout if `-` is given as output file #111626

Conversation

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Overall result: no relevant changes - no action needed

Instruction count

Max RSS (memory usage)

Cycles

Binary size

Recommend

0611 - 信任的建立

昆仑万维真假AI概念迷雾

Private-in-public lints implementation by Bryanskiy · Pull Request #111801 · rus...

fix: fetch nested git submodules by krs98 · Pull Request #12244 · rust-lang/carg...

Slint 1.0 with Tobias Hunger

重大突破：不使用精子和卵子，直接诱导干细胞分化成胚胎！

Wealthiest People in Brazil (June 15, 2023)

Take MIR dataflow analyses by mutable reference by Jarcho · Pull Request #108293...

比亚迪离职潮，车企库存积压的冰山一角

2023年四川省磷化工行业市场现状分析：力争探明新增磷矿储量磷酸铵产品出口规模超3亿...

About Joyk

Write to stdout if `-` is given as output file by pjhades · Pull Request #111626...

Write to stdout if - is given as output file #111626

Conversation

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Overall result: no relevant changes - no action needed

Recommend

About Joyk

Write to stdout if `-` is given as output file #111626