-
Notifications
You must be signed in to change notification settings - Fork 252
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
[rq] ⚠️ upgrade to v0.10.4 #1032
Conversation
Thanks for the pull request! Here is what will happen next:
Thank you for contributing! |
3798003
to
445d716
Compare
As noted by @fnichol during core-plans triage today, despite |
Well, you beat me to this, thanks for updating! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suppose this is still easier than building the tool ourselves?
Thank you so much for diving into that. 🎈
@robbkidd Could you prefix your PRs with the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for digging into this!
I think we've got a bit more work to do regarding the calls to patchelf. I've mentioned the issues inilne.
Feel free to ping me here or in the HabiChat if you have any questions about my comments are think that I may be missing something :D.
rq/plan.sh
Outdated
) | ||
for lib in "${glibc_libs[@]}"; do | ||
build_line "Patching ELF for ${lib}" | ||
patchelf --interpreter "$(pkg_path_for core/glibc)/lib/${lib}" --set-rpath "${LD_RUN_PATH}" "${pkg_prefix}/bin/rq" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this is correct. The --interpreter
flag is setting the INTERP program header. So in this code, you end up setting the interpreter to libgcc_s.so.1 since that is the last call you make down below.
Unfortunately, since that isn't an executable file you'll get:
[46][default:/src:0]# /hab/pkgs/core/rq/0.10.4/20171205110032/bin/rq
bash: /hab/pkgs/core/rq/0.10.4/20171205110032/bin/rq: Permission denied
If you mark libgcc_s as an executable and run it, you'll get a segmentation fault since libgcc_s.so doesn't know how to load the program and presumably starts doing something very wrong.
You should be able to replace lines 31-51 with:
patchelf --interpreter "$(pkg_path_for)/lib/ld-linux-x86-64.so.2" --set-rpath "${LD_RUN_PATH}" "${pkg_prefix}/bin/rq"
The dynamic loader should resolve any needed libraries via the RPATH.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's hugely better, yes. I only pretend to know what I'm doing with elves and patches.
rq/plan.sh
Outdated
# the source tarball contains a cross-compiled binary in a parent directory | ||
# with a vague name. here we'll rename it to the hab standard pkg_dirname | ||
# instead of setting pkg_dirname to this vague name. | ||
mv x86_64-unknown-linux-gnu "$pkg_dirname" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if this is a hab
version difference, but to build this locally I needed to change this to:
mv $HAB_CACHE_SRC_PATH/x86_64-unknown-linux-gnu "$HAB_CACHE_SRC_PATH/$pkg_dirname"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems to be some new behavior in 0.50.3. The working directory of do_unpack()
has historically been $HAB_CACHE_SRC_PATH
(/hab/cache/src
).
I added the following to the beginning of this plan's do_unpack
:
build_line "This is where we're running unpack: $(pwd)"
For the first build in a clean studio, I saw the error and I note that the working directory is /src/rq
.
rq: This is where we're running unpack: /src/rq
rq: Unpacking record-query-v0.10.4-x86_64-unknown-linux-gnu.tar.gz
mv: cannot stat 'x86_64-unknown-linux-gnu': No such file or directory
A second build run immediately after has a different working directory and succeeds:
rq: This is where we're running unpack: /hab/cache/src
rq: Unpacking record-query-v0.10.4-x86_64-unknown-linux-gnu.tar.gz
rq: Setting build environment
So, I can update this plan to be explicit about the full path to the directory to rename. I wonder, though, if that would masking over some new unexpected behavior in the expected working directories of the plan build functions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@robbkidd Yeah, I was almost surely on an older version. I think for core-plans it is OK for us to assume "latest hab" for build time and trust the habitat maintainers to keep the appropriate about of run time backwards compat.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@stevendanna Do you have a recommendation in light of this? I suppose I could wrap the mv
command in a pushd $HAB_CACHE_SRC_PATH ; mv ... ; popd
. But I'm left wondering what changed that the first build has a different working directory for do_unpack
(and maybe other callbacks) than second and subsequent builds.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@robbkidd I am also wondering what changed. I added it to my "hab things to look into" todo list but probably won't get to it until later tomorrow.
For now, I would probably lean towards the thing that will work regardless of the working directly, which is directly using $HAB_CACHE_PATH to reference the source and target directories by absolute paths.
rq/plan.sh
Outdated
} | ||
|
||
do_strip() { | ||
return 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps a comment here to remind us why this turned off would be good. My guess is that we turned this off because of the following patchelf problem: NixOS/patchelf#10
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep.
Or! Things seem to be happy if I move the elf patching to do_strip()
.
47dfd4b
to
2f9e1d6
Compare
Newer rq uses newer toml library which provides more better output when parsing errors occur. With rq v0.9.2 (using [toml v0.2.1](https://github.com/dflemstr/rq/blob/v0.9.2/Cargo.lock#L27)) ``` [ERROR] [rq] Encountered: control character `\n` must be escaped ``` With rq v0.10.4 (using [toml v0.3.1](https://github.com/dflemstr/rq/blob/v0.10.4/Cargo.lock#L472)) ``` [ERROR] [rq] Encountered: newline in string found at line 14 ``` Habitat itself uses rq ([only in one place](https://github.com/habitat-sh/habitat/search?utf8=%E2%9C%93&q=_rq_cmd&type=)) to [pluck out exposed/exported ports from a plan's default.toml](https://github.com/habitat-sh/habitat/blob/1fb090b35943dd013128f49588287a3114dadd06/components/plan-build/bin/shared.sh#L64). This would improve error messaging when there is something awry there. The changes to this plan are mostly around the new way upstream offers their precompiled assets. Names changed. Contents changed. And it's cross-compiled, but now needs some patchelf rubbed upon the build. The elf patching occurs after unneeded symbols have been stripped from the binary because strip does not like the modifications patchelf makes. The dynamic loader should resolve any needed libraries via the RPATH. If there comes a point where the hab-builds-hab primordial soup bootstrap environment does not require rq, we could consider building rq from source, but core would need a v8 engine package for this to depend on to do that. Signed-off-by: Robb Kidd <[email protected]>
2f9e1d6
to
f17d79b
Compare
Updated with edits based on @stevendanna's comments. Squashed into a pretty single commit. Merging is still on hold, pending a review of this working in habitat's primordial soup. |
Nice work! I've pulled this into the base package refresh work but so far from testing it's behaving as expected and we've avoided adding a As mentioned before (though I'm struggling to find the reference) we may end up pulling the job of |
Hello! Wanted to give you an update on this. This pull request has been incorporated into the base plans refresh. We are holding off on merging this refresh until after ChefConf(which takes place May 22-25 in Chicago, IL). If you have any questions, please feel free to ask! We greatly appreciate your work on this and look forward to getting it into master :) |
We've merged #1210 which contains this work. Thanks again!! |
Why?
Newer rq uses newer toml library which provides more better output when parsing errors occur.
Old:
With rq v0.9.2 (using toml v0.2.1)
New:
With rq v0.10.4 (using toml v0.3.1)
Habitat itself uses rq (only in one place) to pluck out exposed/exported ports from a plan's
default.toml. This would improve error messaging when there is something awry there.
How?
The changes to this plan are mostly around the new way upstream offers their precompiled assets. Names changed. Contents changed. And it's cross-compiled, but now needs some patchelf rubbed upon the build.
With all the elves getting patched in this plan, we could consider building rq from source, but core needs a v8 engine package for this to depend on to do that.