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

nheko: 0.6.4 -> 0.7.1 #85922

Merged
merged 12 commits into from May 1, 2020
Merged

nheko: 0.6.4 -> 0.7.1 #85922

merged 12 commits into from May 1, 2020

Conversation

@doronbehar
Copy link
Contributor

doronbehar commented Apr 24, 2020

DONT MERGE, this PR should work once #73940 is merged.

Motivation for this change

https://github.com/Nheko-Reborn/nheko/releases/tag/v0.7.1

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.
@ofborg ofborg bot requested review from Ekleog and fpletz Apr 24, 2020
@doronbehar doronbehar force-pushed the doronbehar:update-nheko branch from 7ee064e to 115661e Apr 24, 2020
@rnhmjoj
Copy link
Contributor

rnhmjoj commented Apr 24, 2020

Oh, it has been released, finally. I had prepared the update but you beat me to it.
I don't remember having problems with boost but I'm on 20.03, here's the commit maybe it can be of help.

@doronbehar
Copy link
Contributor Author

doronbehar commented Apr 24, 2020

One difference I spotted is the use of boost171 vs boost172 ( 17x is used here). I tested with boost171 and still the same error. @rnhmjoj what's the meaning of:

    "-DBoost_NO_BOOST_CMAKE=TRUE"

here?

It fixes the issue but it feels hacky to me considering #73940 ...

@veprbl
Copy link
Member

veprbl commented Apr 24, 2020

@doronbehar Did you try #85254?

@doronbehar
Copy link
Contributor Author

doronbehar commented Apr 24, 2020

@veprbl #85254 works just as well as #73940

@bqv
Copy link
Contributor

bqv commented Apr 24, 2020

I've been using 0.7.0ish for some days, i don't remember having an issue with mtxclient building?

@doronbehar doronbehar force-pushed the doronbehar:update-nheko branch from 115661e to 0e21955 Apr 24, 2020
@bqv
Copy link
Contributor

bqv commented Apr 24, 2020

Looks reasonable compared to the version I'm running, and nothing missing in that inputs list according to @deepbluev7

@bqv
bqv approved these changes Apr 24, 2020
@veprbl
Copy link
Member

veprbl commented Apr 24, 2020

@GrahamcOfBorg build nheko mtxclient tweeny

@bqv
Copy link
Contributor

bqv commented Apr 24, 2020

I've been using 0.7.0ish for some days, i don't remember having an issue with mtxclient building?

Nevermind, I remember it now.

@doronbehar You may find this useful - I'm running and talking with this as we speak. Specifically I think the part that fixes your boost issue is the symlinkJoin
https://github.com/bqv/nixos/blob/live/overlays/nheko.nix

@doronbehar
Copy link
Contributor Author

doronbehar commented Apr 24, 2020

That's no good - I'm pretty sure that your final derivation references both boost.dev and .out - Not good for closure size.

@doronbehar
Copy link
Contributor Author

doronbehar commented Apr 24, 2020

@veprbl the build won't work until either #85254 or #73940 are merged. I don't want to add:

    "-DBoost_NO_BOOST_CMAKE=TRUE"

Which fixes the issue here because that's not a real fix. Please 🙏 consider merging either #85254 or #73940.

@veprbl
Copy link
Member

veprbl commented Apr 24, 2020

@doronbehar I can't just blindly merge any of these and I haven't had time yet to look at those in detail. I propose you use the workaround for now with a comment that it can be removed once #85254 and #73940 are resolved.

@deepbluev7
Copy link

deepbluev7 commented Apr 24, 2020

Btw, you may want to include this commit in your package: Nheko-Reborn/nheko@d94ac86

I accidentally broke room joins in 0.7.x and released 1.7.1 just before I noticed that.

It will take a bit until the next release, that includes this fix.

@doronbehar
Copy link
Contributor Author

doronbehar commented Apr 24, 2020

@doronbehar I can't just blindly merge any of these and I haven't had time yet to look at those in detail. I propose you use the workaround for now with a comment that it can be removed once #85254 and #73940 are resolved.

OK, done.

Btw, you may want to include this commit in your package: Nheko-Reborn/nheko@d94ac86

Thanks @deepbluev7 for joining in and suggesting this, I appreciate your effort you are putting in Nheko.

Unfortunately, and I may need help with this @veprbl , I get this error:

I've applied the patch and everything seems to be OK.

@doronbehar

This comment was marked as resolved.

@doronbehar
Copy link
Contributor Author

doronbehar commented Apr 24, 2020

@GrahamcOfBorg build nheko

@doronbehar doronbehar changed the title [WIP] nheko: 0.6.4 -> 0.7.1 nheko: 0.6.4 -> 0.7.1 Apr 24, 2020
@doronbehar
Copy link
Contributor Author

doronbehar commented Apr 24, 2020

I'm testing the darwin build with:

@GrahamcOfBorg build nheko

@deepbluev7
Copy link

deepbluev7 commented Apr 25, 2020

The build error you get on Darwin looks like your clang is too old to support the deduction guides for std::array. We are only testing clang-6 in our CI. (Not sure, how you guys handle compiler versions.)

@doronbehar doronbehar force-pushed the doronbehar:update-nheko branch 2 times, most recently from 120e0a4 to 83c10ba Apr 25, 2020
@doronbehar
Copy link
Contributor Author

doronbehar commented Apr 25, 2020

OK @deepbluev7 that certainly should be possible to fix but this needs a real Darwin machine. I added a comment that links here.

Besides that all is ready.

@deepbluev7
Copy link

deepbluev7 commented Apr 25, 2020

@bqv clang-6 is just the minimum version, anything newer should work (or even apple clang in Xcode 10.2+). But the CI job used clang-5, which seems to be too old.

@rnhmjoj
Copy link
Contributor

rnhmjoj commented Apr 25, 2020

@bqv That's not that trivial, because we use mkDerivation by qt5... It's too much effort for me to test this, as my only means are GrahamcOfBorg.

libsForQt5.mkDerivation is very simple. Basically you can replicate it just by adding wrapQtAppsHook. See pkgs/development/libraries/qt-5/mkDerivation.nix.

@doronbehar
Copy link
Contributor Author

doronbehar commented Apr 25, 2020

@doronbehar
Copy link
Contributor Author

doronbehar commented Apr 25, 2020

@deepbluev7
Copy link

deepbluev7 commented Apr 25, 2020

@doronbehar The current build fails, because some C++17 library features are only available in macOS 10.14.

@doronbehar
Copy link
Contributor Author

doronbehar commented Apr 25, 2020

@doronbehar The current build fails, because some C++17 library features are only available in macOS 10.14.

Thank you @deepbluev7 for replying. If so, we are almost there but I'm afraid I couldn't find a way to mark it as broken only if the minimum macOS version is not met... Hence I'm marking it as broken for every macOS version for now. If anyone knows a way to do that, please tell me. For now, the PR should be ready.

@doronbehar
Copy link
Contributor Author

doronbehar commented Apr 29, 2020

/ping @veprbl

@veprbl
veprbl approved these changes Apr 30, 2020
Copy link
Member

veprbl left a comment

Passes nixpkgs-review on NixOS, runs, diff looks mostly okay, could use some (light) squashing.

Copy link
Contributor Author

doronbehar left a comment

Thanks @veprbl for catching all of these imperfections. I went through all the commits and made sure they all line up to your comments.

pkgs/top-level/all-packages.nix Outdated Show resolved Hide resolved
@doronbehar doronbehar force-pushed the doronbehar:update-nheko branch from 67e520e to bdcbfc7 Apr 30, 2020
@veprbl veprbl merged commit 35e674f into NixOS:master May 1, 2020
17 checks passed
17 checks passed
Evaluation Performance Report Evaluator Performance Report
Details
grahamcofborg-eval ^.^!
Details
grahamcofborg-eval-check-maintainers matching changed paths to changed attrs...
Details
grahamcofborg-eval-check-meta config.nix: checkMeta = true
Details
grahamcofborg-eval-darwin nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="bdcbfc7"; rev="bdcbfc77c9bda30964d00b41b4c906aa9d489967"; } ./pkgs/t
Details
grahamcofborg-eval-lib-tests nix-build --arg pkgs import ./. {} ./lib/tests/release.nix
Details
grahamcofborg-eval-nixos nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="bdcbfc7"; rev="bdcbfc77c9bda30964d00b41b4c906aa9d489967"; } ./nixos/
Details
grahamcofborg-eval-nixos-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="bdcbfc7"; rev="bdcbfc77c9bda30964d00b41b4c906aa9d489967"; } ./nixos/
Details
grahamcofborg-eval-nixos-options nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="bdcbfc7"; rev="bdcbfc77c9bda30964d00b41b4c906aa9d489967"; } ./nixos/
Details
grahamcofborg-eval-nixpkgs-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="bdcbfc7"; rev="bdcbfc77c9bda30964d00b41b4c906aa9d489967"; } ./pkgs/t
Details
grahamcofborg-eval-nixpkgs-tarball nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="bdcbfc7"; rev="bdcbfc77c9bda30964d00b41b4c906aa9d489967"; } ./pkgs/t
Details
grahamcofborg-eval-nixpkgs-unstable-jobset nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="bdcbfc7"; rev="bdcbfc77c9bda30964d00b41b4c906aa9d489967"; } ./pkgs/t
Details
grahamcofborg-eval-package-list nix-env -qa --json --file .
Details
grahamcofborg-eval-package-list-no-aliases nix-env -qa --json --file . --arg config { allowAliases = false; }
Details
mtxclient, mtxclient.passthru.tests, nheko, nheko.passthru.tests, tweeny, tweeny.passthru.tests on aarch64-linux Success
Details
mtxclient, mtxclient.passthru.tests, nheko, nheko.passthru.tests, tweeny, tweeny.passthru.tests on x86_64-darwin Success
Details
mtxclient, mtxclient.passthru.tests, nheko, nheko.passthru.tests, tweeny, tweeny.passthru.tests on x86_64-linux Success
Details
@DIzFer DIzFer mentioned this pull request May 6, 2020
24 of 37 tasks complete
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
You can’t perform that action at this time.