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

lxd: 3.13 -> 3.18 #70209

Merged
merged 4 commits into from
Oct 15, 2019
Merged

lxd: 3.13 -> 3.18 #70209

merged 4 commits into from
Oct 15, 2019
+104 −16

Conversation

wucke13
Copy link
Contributor

@wucke13 wucke13 commented Oct 1, 2019 •
edited
Loading

Motivation for this change

Hopefully this resolves #69818
Edit:
Which is not the case. But the binaries run fine 😆
Edit 2:
Running this in a nixos-vm, lxd is working!

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)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.
Notify maintainers

cc @fpletz

Sorry, something went wrong.

jonringer
jonringer reviewed Oct 2, 2019
View reviewed changes
Copy link
Contributor

@jonringer jonringer left a comment

Choose a reason for hiding this comment

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

noticed the shared libraries in libco-canonical don't have execute permissions

[nix-shell:/home/jon/.cache/nix-review/pr-70209/results]$ ls -lg libco-canonical/lib/
.r--r--r-- 7.9k root root 31 Dec  1969 libco.a
lrwxrwxrwx   14 root root 31 Dec  1969 libco.so -> libco.so.0.1.0
lrwxrwxrwx   14 root root 31 Dec  1969 libco.so.0 -> libco.so.0.1.0
.r--r--r--  20k root root 31 Dec  1969 libco.so.0.1.0
dr-xr-xr-x    - root root 31 Dec  1969 pkgconfig

[nix-shell:/home/jon/.cache/nix-review/pr-70209/results]$ ls -lg raft-canonical/lib/
.r-xr-xr-x  969 root root 31 Dec  1969 libraft.la
lrwxrwxrwx   16 root root 31 Dec  1969 libraft.so -> libraft.so.0.0.7
lrwxrwxrwx   16 root root 31 Dec  1969 libraft.so.0 -> libraft.so.0.0.7
.r-xr-xr-x 197k root root 31 Dec  1969 libraft.so.0.0.7
dr-xr-xr-x    - root root 31 Dec  1969 pkgconfig

for some closure improvements, i would add outputs = [ "out" "dev" ]; so that the pkgconfig and headers are only used during builds. But the libraries are so small in size it probably won't make too much of a difference.

@wucke13
Copy link
Contributor Author

wucke13 commented Oct 2, 2019

@jonringer Again, excellent review! I'm baffled about the amount detail caught by your eye. I will adjust according to both of your comments in next hours.

@wucke13
Copy link
Contributor Author

wucke13 commented Oct 2, 2019 •
edited
Loading

@jonringer I included your feedback regarding the out thing. Didn't knew that before, thanks for educating me! Regarding the (missing?) x of the .so files, I'm conflicted. A quick research left me here:

  • .so do not have to be executable to be used by linker/ldd
  • some are executable
  • glibcs libc.so.6 for example is executable, for the purpose of outputting some information about itself
  • libco.so.0.1.0 segfaults if ran on its own
  • Funnily, the libraft.so which is executable segfaults when ran too

So my conclusion here is that it would be the best to let the executability flag just as it came from the 3rd party in this case. It probably doesn't brake something and keeping close to upstream feels best.

@jonringer
Copy link
Contributor

Yea, i guess I stumbled upon it becuase i was getting a warning with ldd saying that foo.so is not executable. But if it works in a normal use-case, then I'm 'okay with it being how it is :).

@jonringer
Copy link
Contributor

Unfortunately, I don't have any prior experience with lxd, so I can't really test it well :(

@wucke13
Copy link
Contributor Author

wucke13 commented Oct 2, 2019 •
edited
Loading

Well, you could do me big favor for #69818:
Spin up nixos-vm build from my fork of the nixpkgs with lxd enabled, and run these:

  • initialize lxd: ``lxd init --auto`
  • launch a container: lxc launch images:alpine/3.10

And report the result of your effort in launching the container to the issue #69818:

@wucke13 wucke13 force-pushed the lxd branch 2 times, most recently from 8f2b4bb to eedc510 Compare October 10, 2019 21:15
@wucke13 wucke13 changed the title lxd: 3.13 -> 3.17 lxd: 3.13 -> 3.18 Oct 10, 2019
@wucke13
Copy link
Contributor Author

wucke13 commented Oct 10, 2019 •
edited
Loading

In the meantime a new version of lxd was released, updated accordingly.

Lassulus
Lassulus approved these changes Oct 14, 2019
View reviewed changes
Copy link
Member

@Lassulus Lassulus left a comment

Choose a reason for hiding this comment

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

tested with nixos-generators and this PR checked out as nixpkgs, created an alpine container and looked around it with lxc exec everything looked fine

@Lassulus Lassulus mentioned this pull request Oct 15, 2019
Closed
@Lassulus
Copy link
Member

Do you also want to maintain LXD?

@wucke13
lxd: 3.13 -> 3.18
3e6b5c6 
+ also added myself to maintainer list
@wucke13
Copy link
Contributor Author

wucke13 commented Oct 15, 2019

Do you also want to maintain LXD?

Ok

@Lassulus Lassulus merged commit 04f706e into NixOS:master Oct 15, 2019
@c0bw3b c0bw3b mentioned this pull request Nov 23, 2019
Closed
10 tasks
@mt-caret mt-caret mentioned this pull request Dec 14, 2019
Merged
10 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jonringer jonringer

@Lassulus Lassulus

Assignees
No one assigned
Labels
8.has: package (new) This PR adds a new package 10.rebuild-darwin: 1-10 10.rebuild-linux: 1-10
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Every LXD container fails to start
3 participants
@wucke13 @jonringer @Lassulus