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.

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

fix NOSCRIPTSTUB #10

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

fix NOSCRIPTSTUB #10

wants to merge 2 commits into from

Conversation

lotheac
Copy link
Contributor

@lotheac lotheac commented Apr 8, 2013

Enabling NOSCRIPTSTUB just disables stubbing altogether on master. Merging this should fix that.

@lotheac
Copy link
Contributor Author

lotheac commented Apr 8, 2013

In addition I think enabling NOSCRIPTSTUB should mv files to bin from the arch directories instead of copying them, or you end up having two copies of each script in arch directories which are not likely to be run ever.

@esproul
Copy link
Member

esproul commented Apr 11, 2013

The first change looks fine as a fix for not getting ELF stubs when NOSCRIPTSTUB is non-empty. In thinking about the second change (cp vs. mv) there is actually a deeper problem. If the non-ELF files in i386 vs. amd64 are different, then neither copying nor moving one of them up to the bin/sbin directory is correct.

As an example, see /usr/bin/{i386,amd64}/xml2-config. xml2-config is a shell script, but the content differs because each returns arch-specific build information. Generally speaking, one should explicitly call /usr/bin/i386/xml2-config when used in a 32-bit build, but that may not be possible with every piece of software that wants to run xml2-config. In that case, maybe we do want to create a stub so that I can, for example, do 'ISALIST=i386 ./configure ...' and know that the right version will get run.

Using mv instead of cp is clearly better, but I think the logic should be extended such that if NOSCRIPTSTUB is set:

If the file is non-ELF, and there is either only one copy (BUILDARCH is not "both") or the copies are identical, mv the file to the upper directory.

OR

If the file is non-ELF, but the arch versions differ, make a stub anyway (or possibly do nothing, I'm not sure which is better)

Regardless, I think it's good to fix this, though it is rarely used. Nothing in the core build uses it, and only two things in omniti-ms do. Of those two, one is 32-bit only and therefore doesn't have a problem, but the other is dual-arch and hits the no-stubs-for-binaries issue.

@lotheac
Copy link
Contributor Author

lotheac commented Apr 12, 2013

On Thu, Apr 11 2013 11:38:53 -0700, esproul wrote:

If the file is non-ELF, but the arch versions differ, make a stub
anyway (or possibly do nothing, I'm not sure which is better)

I'd say creating the stub is better here, otherwise anyone who installs
the package won't be able to run the non-ELF very easily.

Lauri Tirkkonen | +358 50 5341376 | lotheac @ IRCnet

@esproul
Copy link
Member

esproul commented Apr 12, 2013

Sounds good to me.

@danmcd
Copy link
Member

danmcd commented May 22, 2014

Hmmm. This is two commits (one for the read(1) fix, and one for the s/cp/mv/). Is that your intention, or do you want them as one commit?

Also, do you care if I push these manually (with your name, etc.) in instead of merging the pull request? Less merge turds if I do the former.

@lotheac
Copy link
Contributor Author

lotheac commented May 22, 2014

On Thu, May 22 2014 12:33:08 -0700, Dan McDonald wrote:

Hmmm. This is two commits (one for the read(1) fix, and one for the
s/cp/mv/). Is that your intention, or do you want them as one commit?

It's your repo, you get to decide what the history looks like. I will
squash them if you want.

Also, do you care if I push these manually (with your name, etc.) in
instead of merging the pull request? Less merge turds if I do the
former.

Doesn't matter terribly much to me either way, but I don't know what you
mean by merge turds.

Lauri Tirkkonen | +358 50 5341376 | lotheac @ IRCnet

@danmcd
Copy link
Member

danmcd commented May 22, 2014

Has this been tested?

@lotheac
Copy link
Contributor Author

lotheac commented May 22, 2014

Yes, in use in our repo niksula/omnios-build for a long time (eg. https://github.com/niksula/omnios-build/blob/master/build/puppet/build.sh)

@lotheac
Copy link
Contributor Author

lotheac commented May 22, 2014

Having said that I should probably implement the additional logic from esproul's comment above before this is merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants