Hi Eli, Thanks for your feedback! My responses can be found below.
2015-11-10 17:51 GMT+01:00 Eli Schwartz <[email protected]>: > On 11/10/2015 10:52 AM, Pieter ROBYNS wrote: > > Dear all, > > > > I have recently created my first PKGBUILD for the tensorflow project > which > > was recently released. This project didn't have a build-from-git AUR > > package yet, so I figured that it would perhaps be useful to other people > > as well and therefore I'd like to share my PKGBUILD on the AUR. > > > > Since it is my first PKGBUILD however, could anyone please give some > > feedback regarding its structure? I also have a few questions: > > > > - The build system used for the project (bazel) appears to require a > > specific version for the build to succeed (the one in AUR currently is > > newer and didn't work out of the box) so I had to clone this version of > the > > build system and compile it temporarily in order to build tensorflow. Is > > this okay given that the same instructions are given on their website ( > > https://tensorflow.org/get_started/os_setup.md#installing_from_sources)? > > They recommend not building from HEAD since it *might* be unstable. > And bazel in the AUR is 0.1.0 which is the version they test against, > and the version your PKGBUILD creates. > > If it wasn't in the AUR already I would recommend you create a package > for it separately. :p Apparently it can be useful... > Yes you are right, it seems the bazel versions do match but for some reason the one from AUR didn't work for me. Could be easily solved with a makedepends entry for bazel. So, best not to upload to AUR? I'm okay with that, I learned something when making the PKGBUILD and now I have a clean bleeding edge package :). > > > - Another requirement by the project is running a ./configure script > before > > building. I did this in prepare(), but the script requires user input. Is > > this a problem, or should it continue automatically? > > ./configure should be part of build() > > prepare() is for things like: > applying patches, > replacing "python" with "python2", > initing git submodules, and configuring them to fetch from a source=() > clone, > > etc. > > Anyway, I know linux-ck allows you to set an option to run `make > nconfig` during prepare() -- same deal with the linux package in core, > except there it needs to be uncommented. > makepkg won't break when requiring user input. But that ideally should > be limited to situations where there are valid choices that need to be > made -- it looks like you can simply export the right answers via an > environment variable. > Okay thanks! I didn't know that. > > > - Finally, I noticed that during build, the CPU usage spikes to 100% > > because bazel parallelizes things. On my machine this caused gcc to crash > > at some point. Do I need to notify the user about this possibility? > > I don't think so. Either they want to build it and they'll live with it, > or they don't. I haven't seen warnings like that for other packages. > I guess it is assumed people will make sure they have enough CPU and RAM > if they want to compile large projects. > I've found an option in bazel to limit the amount of parallel jobs; perhaps that's fine as well. > > > Kind regards, > > Pieter Robyns > > > > You should add protobuf to your source=() array, and fix line 36 to > refer to the "google/protobuf" submodule -- not the "protobuf" submodule. > Ah I see, that probably explains why the protobuf source got cloned and then ignored when I added it to the source array :). > > And why are you using /tmp to create the pip package rather than using a > directory inside the build directory? > > Hehe, I just followed Google's build instructions and I thought it wouldn't matter since their script creates temporary files in /tmp/ anyway. I agree that doing it in the build directory of the package is cleaner. Thanks again for your advice! Kind regards, Pieter
