-
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
icecc-create-env: add command line --addfile "path target" support #548
base: master
Are you sure you want to change the base?
Conversation
…s internally allow non-existent paths to be used for addfile remapping
70863bb
to
514ea0e
Compare
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 feels a bit hackish, both what you need it for and how I assume it needs to be used. How do you actually use this? If you need to explicitly invoke this script this way, for such a workaround, can't you simply unpack the tarball, fix it and repack it?
while [[ $path =~ ([^/][^/]*/\.\./) ]]; do | ||
path=${path/${BASH_REMATCH[0]}/} | ||
done | ||
echo $path |
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.
Why is this part necessary?
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 tested some cases where one might want to remap to a path in the compiler package that doesn't exist on the local system, and therefore the pwd approach would not work. This might be over-reaching now that I look at it again, because I can't think of legitimate reason to do this. Ideally you are trying to make the compiler package match the local system in such a case.
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.
Hm, I would prefer to not be doing this with Bash regexes if possible. They've always been a little odd in the way they work. I think we can just detect that the path doesn't exist here and therefore fail and require an absolute path? Would that be a good option, do you think?
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 would prefer to not be doing this with Bash regexes if possible. They've always been a little odd in the way they work
dunno about the merit of the patch itself, but that argument seems weird to me. the script is written in bash, and we should use its full power when appropriate. there is nothing odd about these regexes; they just have non-posix syntax, as they had to be retro-fitted into the shell syntax.
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.
@ossilator True enough - the argument I was making was more about future readers than the writer. I've used Bash regexes many times before with success, but they tend to confuse people reading them in the future who aren't familiar with them.
In any case, I think overall I would prefer we use a different mechanism for adding paths that don't exist locally anyway and prefer that we use absolute paths with it unless we have a good reason not to.
For example, we want to give the cpuinfo to icecream so we can do ThinLTO incremental linking during the compile remotely, so when it comes back to do the final link all the compiles had the same info as if they were compiled locally. The problem is that we don't want the raw /proc/cpuinfo because it has data in it that is always changing so we end up with a different hash for our compiler package every-time we rebuild. Therefore, we will strip out the data from the cpuinfo with something like Now of course there are workarounds such as repacking, which is fine, maybe not great. I happened to be looking through the icecc-create-env script and noticed the addfile function did support remapping in some form (although maybe I misunderstood it?), but the command line option --addfile did not, so seemed like a simple fix. I am not an icecream expert, but this worked for me when testing. If you think this approach is hackish, I would defer to your experience. I could write it up as an issue instead. |
@@ -485,7 +489,8 @@ if test -n "$clang"; then | |||
# HACK: Clang4.0 and later access /proc/cpuinfo and report an error when they fail | |||
# to find it, even if they use a fallback mechanism, making the error useless | |||
# (at least in this case). Since the file is not really needed, create a fake one. | |||
if test -d /proc; then | |||
# Only add an empty file if the user did not attempt to add there own. |
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 am a bit suspicious of the need to "include" files that don't exist on the source system to satisfy running a binary on a target system. I think we want a more explicit "create a fake file" mechanism.
This allows users to arbitrarily add files that they want to be remapped into some chroot location.
A specific use case is when wanting to remap /proc/cpuinfo from a custom cpuinfo with instantaneous cpu frequency data stripped out for the thinLTO support and the clang bug: https://bugs.llvm.org/show_bug.cgi?id=33008