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

buildRustCrate: Add "-r" to cp to make it work under Mac OS #83488

Merged
merged 1 commit into from Mar 29, 2020
Merged

buildRustCrate: Add "-r" to cp to make it work under Mac OS #83488

merged 1 commit into from Mar 29, 2020

Conversation

kolloch
Copy link
Contributor

@kolloch kolloch commented Mar 27, 2020

Motivation for this change

This fixes errors such as these on Mac OS:

Building src/lib.rs (hello_world_lib_and_bin)
Running rustc --crate-name hello_world_lib_and_bin src/lib.rs --out-dir target/lib --emit=dep-info,link -L dependency=target/deps --cap-lints allow -C debuginfo=2 -C codegen-units=1 --remap-path-prefix=/private/var/folders/vj/wn31skn95g9fz4l9wdm5l5kh0000gn/T/nix-build-rust_hello_world_lib_and_bin-0.1.0.drv-0=/ --cfg feature="default" --edition 2018 -C metadata=06b67cb1f9 -C extra-filename=-06b67cb1f9 --crate-type lib --color always
unpacking sources
unpacking source archive /nix/store/bfrhy6p2dzb8qnixqzhgp2sa3770a69z-ansi_term-0.11.0.tar.gz
unpacking sources
unpacking source archive /nix/store/s8x3gwl0y2a5j4ihxpnnzzpl1v0c9vvi-bitflags-1.2.1.tar.gz
Running rustc --crate-name cfg_test src/main.rs --crate-type bin -C debuginfo=2 -C codegen-units=1 --remap-path-prefix=/private/var/folders/vj/wn31skn95g9fz4l9wdm5l5kh0000gn/T/nix-build-rust_cfg-test-0.1.0.drv-0=/ --cfg feature="default" --edition 2018 --out-dir target/bin --emit=dep-info,link -L dependency=target/deps --extern cfg_test=target/lib/libcfg_test-5198681f4a.rlib --cap-lints allow --color always
cp: -r not specified; omitting directory 'target/bin/hello_world_bin.dSYM'
Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Ran crate2nix tests on
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@kolloch kolloch requested a review from andir as a code owner March 27, 2020 12:13
@andir
Copy link
Member

andir commented Mar 27, 2020

I am not sure this is right. This might fix the issue but it looks like it adds "trash" to $out/bin?

We clearly do not want to have files (or directories) named 'target/bin/hello_world_bin.dSYM' in $out/bin? Am I missing something.

@kolloch
Copy link
Contributor Author

kolloch commented Mar 27, 2020

I don't know. I experimented a bit with omitting .out Directories, it turned out that it was needed. I am not really interested in spending much time on Mac OS but the current version is broken.

@kolloch
Copy link
Contributor Author

kolloch commented Mar 27, 2020

DSYM files might make sense. They contain debugging symbols and might be useful for better stack traces.

@Mic92 Mic92 requested a review from LnL7 March 27, 2020 18:16
@LnL7
Copy link
Member

LnL7 commented Mar 27, 2020 •
edited

It's not really clear to me what the actual "error" is here.

Packages in nixpkgs (should) run release builds and are striped by default. If you want this for debugging purposes, handling this with a debug output, similar to what seprateDebugInfo = true; does on linux for dwarf symbols would be the way to go.

@andir
Copy link
Member

andir commented Mar 27, 2020 via email

@LnL7
Copy link
Member

LnL7 commented Mar 27, 2020

Disabling whatever causes the separate debug symbols to be created would be the best fix for now I think (I notice -C debuginfo=2 in the output).

That said having proper support for debug info on darwin would be great, but that's probably a bit out of scope for this.

@LnL7
Copy link
Member

LnL7 commented Mar 27, 2020 •
edited

Seems like just moving it works so adding a setup-hook similar to separateDebugInfo should be pretty straightforward.

https://gist.github.com/LnL7/506e719a5fa339520877acecd4fe3a9f

@kolloch kolloch force-pushed the unfancy-macos-cp-r branch 3 times, most recently from e8c8bcb to 4e4490d Compare March 28, 2020 14:56
@andir andir mentioned this pull request Mar 28, 2020
Merged
10 tasks
@kolloch kolloch force-pushed the unfancy-macos-cp-r branch 2 times, most recently from bc316ea to 7e4353a Compare March 28, 2020 15:51
andir
andir reviewed Mar 28, 2020
View reviewed changes
pkgs/build-support/rust/build-rust-crate/test/default.nix
@@ -94,6 +94,44 @@ let
''
);

assertOutputs = { name, crateArgs, expectedFiles, output? null }:
Copy link
Member

Choose a reason for hiding this comment

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

I think we should start adding some "docstring"-like comments to these function (as we do in crate2nix). During the discussion of #83379 I was told that currently it isn't really obvious how they work. I intend to refactor the test runner once all the current PRs have been merged and things have settled down.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am all for docs! I added some (push still outstanding).

@kolloch kolloch force-pushed the unfancy-macos-cp-r branch 2 times, most recently from 69ccaa7 to 6ddf45e Compare March 28, 2020 16:55
@kolloch
Copy link
Contributor Author

kolloch commented Mar 28, 2020

Seems like just moving it works so adding a setup-hook similar to separateDebugInfo should be pretty straightforward.

https://gist.github.com/LnL7/506e719a5fa339520877acecd4fe3a9f

I tried to do that but it didn't work for me. E.g. on Mac OS compgen -G or ${debug:-out}. Does it use a different shell? Annoying. The error might be mine but this is a timeout for me.

I found out that the dSYM files are only generated if release = false is specified to buildRustCrate. I think that is totally acceptable. Furthermore, we added additional files to the output before (e.g. dependency files which I removed now) which nobody noticed before.

@ofborg ofborg bot requested a review from andir March 28, 2020 17:20
@kolloch
Copy link
Contributor Author

kolloch commented Mar 28, 2020

And I am getting an unrelated error on Mac OS (I was quite confused because I first thought that it was introduced by my changes):

https://gist.github.com/kolloch/311487d7b960f4a79f57c13926635cb7

@kolloch
Copy link
Contributor Author

kolloch commented Mar 28, 2020

When I revert the two linkorder PRs, it works:

peter@dipkus: ~/nixpkgs$ git revert d8b853799d6658582a6039be8febfbd925d967aa
[master 653a421b60e] Revert "buildRustCrate: don't sort link flags"
 2 files changed, 6 insertions(+), 1 deletion(-)

peter@dipkus: ~/nixpkgs$ git revert 2f7fb1c49732105c8937b66b7d92402c04546073
[master 49cc1eda0ea] Revert "buildRustCrateTests: add regression test for link order"
 1 file changed, 1 insertion(+), 53 deletions(-)

peter@dipkus: ~/nixpkgs$ nix-build -k -A buildRustCrateTests
/nix/store/jg9p6p7zz66pfj4vf3cv71sbxjiiq34v-buildRustCrate-tests

(Which is kind of a noop because I also delete the relevant test)

@andir
Copy link
Member

andir commented Mar 28, 2020

Sorry for that. I fixed those and opened a PR for review: #83616

@kolloch
Copy link
Contributor Author

kolloch commented Mar 28, 2020

No worries, @andir! "Wo gehobelt wird, da Fallen Spaene."

Back to this PR. Given

  1. that the problem only appears with release = false
  2. and I made thinks better by actually adding tests that ensure that no random junk files pop up for binaries and rlibs,

I think that the PR is in reasonable shape now. If you have any actionable feedback, let me now.

@kolloch @andir
buildRustCrate: Add tests for checking files in outputs.
782b304 
...and remove superfluous dependency files (*.d).
...and copy dSYM directories on Mac OS when in release=false mode.
andir
andir approved these changes Mar 29, 2020
View reviewed changes
Copy link
Member

@andir andir left a comment

Choose a reason for hiding this comment

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

Tested on Linux & MacOS.

MacOS did pass after I rebased this onto master. 👍

Thank you for fixing this.

@andir andir merged commit 69e8d49 into NixOS:master Mar 29, 2020
@andir andir mentioned this pull request Mar 29, 2020
Merged
3 tasks
LnL7 added a commit to LnL7/ofborg that referenced this pull request Apr 25, 2020
@LnL7
disable debug build on darwin
5669d9b 
Currently broken due to the .dSYM folders it generates next to the binaries.

See NixOS/nixpkgs#83488
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@andir andir andir approved these changes

@LnL7 LnL7 Awaiting requested review from LnL7

Assignees
No one assigned
Labels
6.topic: rust 10.rebuild-darwin: 0 10.rebuild-linux: 1-10
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@kolloch @andir @LnL7