Add: XAPI (or Xenserver or XCP) + dependencies #5529
Conversation
ts468
force-pushed
the
xapi
branch
6 times, most recently
from
2aa81e1 to
d4c37ca
Compare
|
Where do files like |
| ### END INIT INFO | ||
|
|
||
| # Source function library. | ||
| . /lib/lsb/init-functions |
|
@peti, thanks for having a look at the PR! So far Xenserver is only provided as ISO images, and it is currently being broken up into individual packages. Currently the Xenserver packages are only targeting Redhat based systems, Debian based systems are to follow. The build of the packages is done through a script on https://github.com/xenserver/buildroot . That repository is also the source of the additional files in the PR. What I did was to take the RPM .spec files from https://github.com/xenserver/buildroot , and converted them into nix expressions. The additional files that you are asking about were copied blindly as I was just trying to get everything into a state where Xenserver/Xapi would compile. Some of the additional files are Redhat specific init scripts and won't work on NixOS, you're right. I intend to sort that out while writing the nixos modules for Xapi/Xenserver. |
|
I agree with @peti, it would be much better to get all this stuff using fetchurl or fetchgit or something similar. Also, please squash or rebase so this giant commit doesn't end up in the the repository's history; the size is nontrivial. (Thanks for the hard work!) |
|
@falsifian, thanks for your encouraging feedback! In a local repository I've already replaced the fetchurl/fetchgit parts of the commit. The init scripts for the XAPI tool chain will have to be fixed later on though. With this PR I will just try to get the packages in place. I'm just waiting for #5995 and #6046 to be merged before I rebase this one. Of course it would be great if you could help me with that two PRs first. :) |
|
It looks like @edolstra is commenting on the remaining pull request (which is good, because I don't know anything about Xen :-). |
ts468
force-pushed
the
xapi
branch
2 times, most recently
from
6b3e427 to
4f161b8
Compare
|
PR was rebased, now that #6046 has been merged. @falsifian and @peti, could you maybe review and reconsider to merge the PR? |
|
Personally, I don't feel qualified to decide / merge this PR, because I don't know much about Xen or OCaml. |
|
Style note:
|
| @@ -12028,11 +12182,122 @@ let | |||
|
|
|||
| xdotool = callPackage ../tools/X11/xdotool { }; | |||
|
|
|||
| xenserverBuildroot = callPackage ../applications/virtualization/xenserver { }; | |||
There was a problem hiding this comment.
Why is the package called xenserver but the attribute xenserverBuildRoot?
|
It looks like this commit touches a great number of ocaml-related files. Are these really part of this commit? Or did something go wrong with the rebase? |
ts468
force-pushed
the
xapi
branch
2 times, most recently
from
404c03f to
08c6800
Compare
|
@edolstra, thanks for reminding me not to use camelCase, I've changed it accordingly. @falsifian, the commit touches 4 existing ocaml-files. That are intended version updates. Thanks for checking! Anything else that I should look at before the PR can be merged? |
|
@ts468 in the "Files Changed" tab I see many more than four OCaml files. E.g. ocaml-async-extra, ocaml-async-find, ocaml-async-inotify, ocaml-async-kernel, ocaml-async-unix, ocaml-async, ocaml-backtrace, ocaml-bin-prot, ocaml-cdrom, .... The total number of files changed is 117, and a not-so-small fraction of that seems to be ocaml-related. (I haven't actually read much of the PR, I'm afraid.) |
|
@falsifian, XAPI is mainly written in OCaml, and so the files are often libraries or packages that are needed by XAPI, so they are all intended to be in the PR. ;) |
| ocaml_rrdd-plugin = callPackage ../development/ocaml-modules/ocaml-rrdd-plugin { xen = xen_xenServer; }; | ||
|
|
||
| ocaml_xenops = callPackage ../development/ocaml-modules/ocaml-xenops { xen = xen_xenServer; }; | ||
|
|
There was a problem hiding this comment.
Minor nit: prefixing all of these names with ocaml_ seems redundant, since they're already inside mkOcamlPackages. Also it is inconsistent with the surrounding names.
|
In that case, thanks for doing all this hard work! I can't promise I'll be able to look it all over myself :-( @edolstra, have you looked through much of it? What is the standard for merging a big thing like this? Calling @vbgl to verify that the ocaml stuff makes sense. (If you have time, please take a look or pass it on to another person who knows about ocaml in nixpkgs; I just picked you because you've committed a bunch of stuff.) |
|
What a large work! Amazing. Is it necessary to have only one large commit? It would be easier to There is an evaluation error in I agree with @falsifian: the attributes inside ocamlPackages need not be The URL of ocaml-text sources is wrong. Some packages are already in nixpkgs. They might have been added recently:
|
|
@falsifian and @vbgl, thank you very much for reviewing the PR! I hope the changes can make it upstream soon. Concerning the naming, I had problems figuring out the pattern that was used so far for package names, attributes and file names. It seems as a ocaml package is added without prefix in case it does not clash with any other package name, and prefixed with ocaml_ in case a package of the same name already existed in nixpkgs---or somehow like that, e.g. look at the ocaml module for cairo. What I would suggest is the following for a package "bar": For example, for the cairo ocaml module that would be: I would reserve a character in the name of a package just for representing the hierarchical structure the package is in---maybe there is already a general character in use for that in nixpkgs. For example, I picked "_" for that, but it could be "@" or "/" or "." or any other character that is "reserved" for just that one purpose. The xapi-storage vs. xapiStorage error probably slipped in when I removed the camelCase naming recently, thanks! Also, thanks for pointing me towards the ocaml-text URL issue. |
|
If I can add a general remark, it would help me if a PR could be merged faster, and if the PR is then being fixed upstream. If a PR is expected not to break stuff, a faster merge would avoid the cumbersome rebasing of the PR to upstream changes that affect it. Also, I guess for someone with direct commit access, it's sometimes easier to just fix stuff instead of going through the review-comment-change-review loop all the time. Splitting a large PR into several smaller pieces would just increase the overhead for maintenance, I think, especially if they all need to be reviewed independently. For example for that PR, it would result in over a 100 individual PRs that I would have to ask people to review, and that I would have to manage. Lastly, if my further work depends on the changes of a pending, yet to be merged PR, then I personally quickly run into a git-branch nightmare. While working on a larger project, it seems like I always end up with circular dependencies between the git branches. So if I could get stuff upstream faster, it would just help me to avoid the time intense git-branch disentanglement. Although it's a far stretch, but I guess the findings of the differential synchronization algorithm (https://neil.fraser.name/writing/sync/) also apply here---high frequency, small changes just reduce the overall work to be done (https://www.youtube.com/watch?v=S2Hp_1jqpY8 47:00). Don't get me wrong, I do not want to complain! I really appreciate all your efforts with integrating my changes, and your enormous work on NixOS in general! It is just a thought ;). |
|
Consistent naming in nixpkgs would be nice. Probably a topic for the mailing list. Sorry for the slow review process. I agree with @vbgl that it would be easier to review in smaller pieces. It doesn't have to be 100 different PRs... e.g. it's probably fine to stick all the new OCaml modules in one PR, if they're all similar to each other. To be honest I haven't yet taken the time to learn how many different kinds of thing this PR does (partly due to laziness) which would be the first step for me to really review this. I think a PR that just does one kind of thing (add new OCaml modules, even if it's 30 of them at once) would be easier to review. Fixing little things after merging is probably fine, but I haven't found the time to read the PR yet except for little pieces :-( |
|
Sorry for the very long delay. I don't want the PR to be lost, so here is another attempt to bring it upstream ;) I've split the PR:
Please let me know if you encounter any evaluation or build errors, or if there are other changes that are required before merging the PR upstream. Many thanks in advance! |
|
Wow, thanks for the work splitting it up! I can't promise I'll be able (or qualified) to review all these, but I'll try to take a look. |
|
Is this still being worked on? I'd be happy to merge it, looks cool. Seems like travis failed with a legitimate error, though: |
peti
added
0.kind: enhancement
|
would you ,@ts468 (or somebody else), be interested in bringing this PR up to date with master? |
|
Seems like this hasn't gotten any attention in a while. @ts468 is this still on your radar? |
|
Closing due to 3 bumps. Happy to reopen :) |
That PR brings in XAPI, or Xenserver or XCP if you want to call it like that.
XAPI is build out of a lot of individual packages and ocaml dependencies, and the PR contains all of them.
The PR should be safe to merge as it only upgrades three ocaml packages and adds a ton of new packages.
Every package built at least once on my local machine.
XAPI is not expected to be usable yet. The packages might have to be adapted or fixed when the nixos modules are written.
I would appreciate an early merge to allow more people to start playing with the XAPI tools. I also intend to invite the people behind XAPI/Xenserver to have a look at the NixOS integration once the raw packages have been merged.