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

@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

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

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

@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

@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

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

@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

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 has been minimized.

@doronbehar
Copy link
Contributor Author

@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

I'm testing the darwin build with:

@GrahamcOfBorg build nheko

@deepbluev7
Copy link

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 update-nheko branch 2 times, most recently from 120e0a4 to 83c10ba Compare April 25, 2020 00:34
@doronbehar
Copy link
Contributor Author

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

@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

@GrahamcOfBorg build

@doronbehar
Copy link
Contributor Author

The Darwin build still fails, but differently:

https://logs.nix.ci/?key=nixos/nixpkgs.85922&attempt_id=1d08c688-e7d7-45c0-bb00-c25f49ce9651

@deepbluev7
Copy link

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

@doronbehar
Copy link
Contributor Author

@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

/ping @veprbl

Copy link
Member

@veprbl veprbl left a comment

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@doronbehar doronbehar left a comment

Choose a reason for hiding this comment

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

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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants