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

rocksdb: 5.11.3 -> 6.1.2 #58834

Merged
merged 2 commits into from
Jun 19, 2019
Merged

rocksdb: 5.11.3 -> 6.1.2 #58834

merged 2 commits into from
Jun 19, 2019

Conversation

magenbluten
Copy link
Contributor

Motivation for this change

rocksdb was out of date. also removed stuff that seemed broken.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • 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 nix-review --run "nix-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)
  • Assured whether relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@ryantm
Copy link
Member

ryantm commented Apr 4, 2019

cc @adevress

ryantm
ryantm requested changes Apr 4, 2019
View reviewed changes
@ryantm
Copy link
Member

ryantm commented Apr 4, 2019

@GrahamcOfBorg build rocksdb

@infinisil infinisil added the 8.has: package (update) This PR updates a package to a newer version label Apr 4, 2019
ryantm
ryantm requested changes Apr 5, 2019
View reviewed changes
@magenbluten magenbluten changed the title rocksdb: 5.11.3 -> 5.18.3 rocksdb: 5.11.3 -> 6.0.1 Apr 17, 2019
Mic92
Mic92 reviewed Apr 17, 2019
View reviewed changes
Mic92
Mic92 approved these changes Apr 17, 2019
View reviewed changes
Copy link
Member

@Mic92 Mic92 left a comment

Choose a reason for hiding this comment

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

Looks good to me otherwise.

Mic92
Mic92 reviewed Apr 17, 2019
View reviewed changes
pkgs/development/libraries/rocksdb/default.nix
(lib.optional
(stdenv.hostPlatform.system == "i686-linux"
|| stdenv.hostPlatform.system == "x86_64-linux")
"-DFORCE_SSE42=1")
Copy link
Member

Choose a reason for hiding this comment

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

Could this break somewhere?

@ofborg ofborg bot requested review from cstrahan and Ma27 April 29, 2019 06:59
@magenbluten magenbluten changed the title rocksdb: 5.11.3 -> 6.0.1 rocksdb: 5.11.3 -> 6.0.2 Apr 30, 2019
@Lassulus
Copy link
Member

Lassulus commented May 7, 2019

@GrahamcOfBorg build ceph

@Lassulus
Copy link
Member

Lassulus commented May 7, 2019

this also breaks ceph. and everything that depends on it. But ceph seems to be an old version? @adevress @alexanderkjeldaas

@Lassulus
Copy link
Member

since no one responded for the ceph breakage, we marked ceph as broken.

@ofborg ofborg bot requested a review from Ma27 May 21, 2019 21:34
flokli
flokli reviewed May 22, 2019
View reviewed changes
Copy link
Contributor

@flokli flokli left a comment

Choose a reason for hiding this comment

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

some nitpicks.

@magenbluten magenbluten changed the title rocksdb: 5.11.3 -> 6.0.2 rocksdb: 5.11.3 -> 6.1.1 Jun 5, 2019
@magenbluten magenbluten changed the title rocksdb: 5.11.3 -> 6.1.1 rocksdb: 5.11.3 -> 6.1.2 Jun 18, 2019
@magenbluten
Copy link
Contributor Author

magenbluten commented Jun 18, 2019 •
edited
Loading

i tried to unbreak osquery with no luck. it still fails when linking against rocksdb(-lite). see https://gist.github.com/magenbluten/997142eaa828ccd177c0dd06cc8261b4

@Lassulus
Copy link
Member

@Ma27 any points against merging this? since you recently updated osquery and this PR would break it.

@Ma27
Copy link
Member

Ma27 commented Jun 18, 2019

No. While working on osquery quite recently I realized that they currently have a hard time staying up-to-date with new software versions and I don't consider osquery important enough to actually block this PR any longer.

Therefore, it's IMHO ok to merge this now, I'll take care of osquery on one of the next weekends :)

rocksdb: 5.11.3 -> 6.1.2
e369007 
- mark osquery as broken
- mark ceph as broken

both osquery and ceph packages are outdated. furthermore, ceph has its
own inline rocksdb source tree which isn't use in the current nixpkg.
this needs to be fixed.
@Lassulus Lassulus merged commit b54b5f9 into NixOS:master Jun 19, 2019
@Ma27
Copy link
Member

Ma27 commented Jun 29, 2019

@magenbluten @Lassulus I managed to get osquery building locally (didn't test ceph as a bunch of python2 deps appear to be broken on master atm) and it works perfectly fine when using the old build system (no cmake). I suspect this is because the cmake build doesn't create statically linked libraries.

According to the install section it's recommended to simply use make {shared,static}_lib depending on what's needed.

I'd simply go back to the old build approach for rocksdb (and keep 6.1.2), however I'm not sure if I'm missing a reason why cmake is used now (and I probably just didn't manage to get the static lib building with cmake ^^).

Do you think using the old make-based approach makes sense or am I missing something? :)

@magenbluten
Copy link
Contributor Author

i guess the install section you mention is outdated (dez. 2018).

the "correct" solution would be to figure out how to make cmake make static libs ;)

@magenbluten
Copy link
Contributor Author

magenbluten commented Jun 30, 2019 •
edited
Loading

git grep -i static_lib:

CMakeLists.txt:set(ROCKSDB_STATIC_LIB rocksdb${ARTIFACT_SUFFIX})

seems like it should work with cmake.

@infinisil infinisil mentioned this pull request Nov 24, 2019
Merged
10 tasks
@Janik-Haag Janik-Haag added the 12. first-time contribution This PR is the author's first one; please be gentle! label Jun 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Mic92 Mic92 Mic92 approved these changes

@flokli flokli flokli left review comments

@Lassulus Lassulus Lassulus left review comments

@ryantm ryantm ryantm requested changes

@cstrahan cstrahan Awaiting requested review from cstrahan

@Ma27 Ma27 Awaiting requested review from Ma27

Assignees
No one assigned
Labels
8.has: clean-up 8.has: package (update) This PR updates a package to a newer version 10.rebuild-darwin: 1-10 10.rebuild-linux: 11-100 11.by: package-maintainer This PR was created by the maintainer of the package it changes 12. first-time contribution This PR is the author's first one; please be gentle!
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@magenbluten @ryantm @Lassulus @Ma27 @Mic92 @flokli @infinisil @GrahamcOfBorg @Janik-Haag