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

libdnf: switch back to default stdenv. #138540

Merged
merged 1 commit into from
Sep 19, 2021
Merged

Conversation

rb2k
Copy link
Contributor

@rb2k rb2k commented Sep 19, 2021

Motivation for this change

The only reason this was on GCC was because of some implicit constructor conversion issues that gcc handled without complaining and clang did not.

This commit fixed those:
rpm-software-management/libdnf@fefe0b6

Now that it works on clang, might as well revert back to the default.

Note: I can't currently test this on darwin because libbfd in master seem broken even without my modifications. I just end up with this: https://www.internalfb.com/phabricator/paste/view/P459437194

I did however apply the fefe0b69e8368a8e9ceeed107add174c64f00f5e commit as a patch to 21.05 and was able to also compile it with stdenv.

Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all packages 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/)
  • 21.11 Release Notes (or backporting 21.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

@alyssais
Copy link
Member

This commit message is misleading — what this change does is switch back to the default stdenv, which on most platforms is the gcc stdenv, not the clang one. Could you rephrase it?

@rb2k
Copy link
Contributor Author

rb2k commented Sep 19, 2021

This commit message is misleading — what this change does is switch back to the default stdenv, which on most platforms is the gcc stdenv, not the clang one. Could you rephrase it?

Yeah, you're right. That's better. PR is still in draft form and not quite ready, so I'll add some more details over the next few minutes :)

@rb2k rb2k changed the title libdnf: swtich to clang. libdnf: switch back to default stdenv. Sep 19, 2021
@rb2k rb2k force-pushed the libdnf_clang_master branch from 65194cc to cd9a774 Compare September 19, 2021 15:54
@rb2k
Copy link
Contributor Author

rb2k commented Sep 19, 2021

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

2 packages built:
  • libdnf
  • microdnf

@rb2k rb2k force-pushed the libdnf_clang_master branch from cd9a774 to 31b5fa9 Compare September 19, 2021 15:55
@rb2k rb2k marked this pull request as ready for review September 19, 2021 15:55
@rb2k rb2k requested a review from alyssais September 19, 2021 15:58
@alyssais
Copy link
Member

The commit message still doesn't make sense — the version you changed the PR title to is good, can you use that as the commit message too?

@rb2k rb2k force-pushed the libdnf_clang_master branch from 31b5fa9 to e7f64e6 Compare September 19, 2021 16:02
@rb2k rb2k force-pushed the libdnf_clang_master branch from e7f64e6 to 5c5db1f Compare September 19, 2021 16:02
@rb2k
Copy link
Contributor Author

rb2k commented Sep 19, 2021

The commit message still doesn't make sense — the version you changed the PR title to is good, can you use that as the commit message too?

ugh, sorry. clearly I need more coffee this morning :)
done

@alyssais
Copy link
Member

@ofborg build libdnf.override { stdenv = clangStdenv; }

@alyssais
Copy link
Member

Hmm, that appeared not to work. Can I get it to work like this?

@ofborg build libdnf.override{stdenv=clangStdenv;}

@rb2k
Copy link
Contributor Author

rb2k commented Sep 19, 2021

Found the issue with darwin on master: #138545 (indeed unrelated to this)

Copy link
Member

@alyssais alyssais 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 x86_64-linux with

nix-build -E 'with import <nixpkgs> {}; libdnf.override { stdenv = clangStdenv; }' -I nixpkgs=https://github.com/NixOS/nixpkgs/tarball/pull/138540/head

@alyssais alyssais merged commit 73414b5 into NixOS:master Sep 19, 2021
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.

2 participants