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.

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

mrsh: 2020-01-08 -> 2020-07-27 etc. #101910

Merged
merged 1 commit into from Nov 2, 2020
Merged

Conversation

@omasanori
Copy link
Contributor

@omasanori omasanori commented Oct 28, 2020

Motivation for this change

This change updates mrsh to a newer revision. The changes between them are available.

Moreover, it enables doCheck as executing the whole test suites takes just few seconds.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • 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.
@omasanori
Copy link
Contributor Author

@omasanori omasanori commented Oct 28, 2020

ping @matthiasbeyer (as ofborg did not set a reviewer for some reason.)

Copy link
Contributor

@matthiasbeyer matthiasbeyer left a comment

diff LGTM

@SuperSandro2000
Copy link
Member

@SuperSandro2000 SuperSandro2000 commented Oct 28, 2020

Result of nixpkgs-review pr 101910 1 run on x86_64-darwin

1 package failed to build:
  • mrsh
Library readline found: YES
Checking for function "rl_replace_line" with dependency -lreadline: YES
Checking if "-Wl,--version-script" links: NO
Compiler for C supports link arguments -Wl,-exported_symbols_list /private/tmp/nix-build-mrsh-2020-07-27.drv-0/source/libmrsh.darwin.sym: NO

meson.build:71:1: ERROR: Problem encountered: Linker doesn't support --version-script or -exported_symbols_list

A full log can be found at /private/tmp/nix-build-mrsh-2020-07-27.drv-0/source/build/meson-logs/meson-log.txt

@omasanori
Copy link
Contributor Author

@omasanori omasanori commented Oct 30, 2020

@SuperSandro2000 Perhaps it assumes that the compiler is Clang on macOS: https://github.com/emersion/mrsh/blob/0da902c0ee6f443fe703498e60f266af7f12537e/meson.build#L67-L70 I have no access to macOS so it is just a guess though.

@omasanori omasanori force-pushed the mrsh/update-2020-07-27 branch from 1f13c0b to 7004b98 Oct 30, 2020
@omasanori
Copy link
Contributor Author

@omasanori omasanori commented Oct 30, 2020

@SuperSandro2000 Would you mind if I ask for retest on macOS?

@omasanori omasanori changed the title mrsh: 2020-01-08 -> 2020-07-27, enable check mrsh: 2020-01-08 -> 2020-07-27 etc. Oct 30, 2020
@SuperSandro2000
Copy link
Member

@SuperSandro2000 SuperSandro2000 commented Oct 31, 2020

@SuperSandro2000 Perhaps it assumes that the compiler is Clang on macOS: emersion/mrsh@0da902c/meson.build#L67-L70 I have no access to macOS so it is just a guess though.

I am not 100% sure but I think so.

@SuperSandro2000 Would you mind if I ask for retest on macOS?

Not at all. Added to the list.

@SuperSandro2000
Copy link
Member

@SuperSandro2000 SuperSandro2000 commented Oct 31, 2020

Result of nixpkgs-review pr 101910 run on x86_64-linux 1

1 package built:
  • mrsh

@SuperSandro2000
Copy link
Member

@SuperSandro2000 SuperSandro2000 commented Oct 31, 2020

Still failing for darwin:

Host machine cpu family: x86_64
Host machine cpu: x86_64
Compiler for C supports arguments -Wundef: YES
Compiler for C supports arguments -Wlogical-op: NO
Compiler for C supports arguments -Wmissing-include-dirs: YES
Compiler for C supports arguments -Wold-style-definition: YES
Compiler for C supports arguments -Wpointer-arith: YES
Compiler for C supports arguments -Winit-self: YES
Compiler for C supports arguments -Wfloat-equal: YES
Compiler for C supports arguments -Wstrict-prototypes: YES
Compiler for C supports arguments -Wredundant-decls: YES
Compiler for C supports arguments -Wimplicit-fallthrough=2: NO
Compiler for C supports arguments -Wendif-labels: YES
Compiler for C supports arguments -Wstrict-aliasing=2: YES
Compiler for C supports arguments -Woverflow: YES
Compiler for C supports arguments -Wformat=2: YES
Compiler for C supports arguments -Wmissing-prototypes: YES
Compiler for C supports arguments -Wno-missing-braces: YES
Compiler for C supports arguments -Wno-missing-field-initializers: YES
Compiler for C supports arguments -Wno-unused-parameter: YES
Compiler for C supports arguments -Wno-unused-result: YES
Compiler for C supports arguments -Wno-format-overflow: NO
Library readline found: YES
Checking for function "rl_replace_line" with dependency -lreadline: YES
Checking if "-Wl,--version-script" links: NO
Compiler for C supports link arguments -Wl,-exported_symbols_list /private/tmp/nix-build-mrsh-2020-07-27.drv-0/source/libmrsh.darwin.sym: NO

meson.build:71:1: ERROR: Problem encountered: Linker doesn't support --version-script or -exported_symbols_list

A full log can be found at /private/tmp/nix-build-mrsh-2020-07-27.drv-0/source/build/meson-logs/meson-log.txt

omasanori added a commit to omasanori/nixpkgs that referenced this issue Oct 31, 2020
- Since it takes just a few seconds, `doCheck` is enabled.
- To fix [build failures on macOS][macos-issue], GCC stdenv is forced.

[macos-issue]: NixOS#101910 (comment)

Reference: https://github.com/emersion/mrsh/compare/ef21854fc9..0da902c0ee
Signed-off-by: Masanori Ogino <167209+omasanori@users.noreply.github.com>
@omasanori omasanori force-pushed the mrsh/update-2020-07-27 branch from 7004b98 to 7203ff1 Oct 31, 2020
@omasanori
Copy link
Contributor Author

@omasanori omasanori commented Oct 31, 2020

So now I am trying the opposite, forcing GCC to avoid the problem...
Please test it on macOS again when you have time.

@SuperSandro2000
Copy link
Member

@SuperSandro2000 SuperSandro2000 commented Nov 1, 2020

Result of nixpkgs-review pr 101910 run on x86_64-darwin 1

1 package failed to build:
  • mrsh
Compiler for C supports arguments -Wno-unused-parameter -Wunused-parameter: YES
Compiler for C supports arguments -Wno-unused-result -Wunused-result: YES
Compiler for C supports arguments -Wno-format-overflow -Wformat-overflow: YES
Library readline found: YES
Checking for function "rl_replace_line" with dependency -lreadline: YES
Checking if "-Wl,--version-script" links: NO
Compiler for C supports link arguments -Wl,-exported_symbols_list /private/tmp/nix-build-mrsh-2020-07-27.drv-0/source/libmrsh.darwin.sym: NO

meson.build:71:1: ERROR: Problem encountered: Linker doesn't support --version-script or -exported_symbols_list

A full log can be found at /private/tmp/nix-build-mrsh-2020-07-27.drv-0/source/build/meson-logs/meson-log.txt

@omasanori
Copy link
Contributor Author

@omasanori omasanori commented Nov 1, 2020

Thanks a lot! Hmm, these linker options must not be such brand-new ones, so I suspected compilers first. However, it seems that a more detailed analysis is needed...

@SuperSandro2000
Copy link
Member

@SuperSandro2000 SuperSandro2000 commented Nov 1, 2020

@omasanori I would be fine with disabling it on darwin for now.

@omasanori
Copy link
Contributor Author

@omasanori omasanori commented Nov 1, 2020

@SuperSandro2000 I guess so. @matthiasbeyer, is it acceptable to you?

@matthiasbeyer
Copy link
Contributor

@matthiasbeyer matthiasbeyer commented Nov 1, 2020

No objections from me.

- `doCheck` is enabled now.
- It marked as broken on Darwin due to [build failures][macos-issue].

[macos-issue]: NixOS#101910 (comment)

Reference: https://github.com/emersion/mrsh/compare/ef21854fc9..0da902c0ee
Signed-off-by: Masanori Ogino <167209+omasanori@users.noreply.github.com>
@omasanori omasanori force-pushed the mrsh/update-2020-07-27 branch from 7203ff1 to 955926a Nov 1, 2020
@nixos-discourse
Copy link

@nixos-discourse nixos-discourse commented Nov 2, 2020

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/prs-already-reviewed/2617/271

@kevincox kevincox merged commit 22409c8 into NixOS:master Nov 2, 2020
19 checks passed
@omasanori omasanori deleted the mrsh/update-2020-07-27 branch Nov 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants