Skip to content

Conversation

@StudioEtrange
Copy link
Contributor

Ok, then
First PR in my attempt to merge my stuff upstream.
This PR is about 3 new options

I made 3 commits, one for each option, and 1 more for help and readme

--no-header which do not print binary info and its interpreter

--no-recursive to not parse dependencies of dependencies

-b to force use of a specific backend tools (so no, auto fallback)

@StudioEtrange
Copy link
Contributor Author

StudioEtrange commented Jul 14, 2016

For binutils, why do you use objdump AND readelf ?
I think its better to rely on one tool only. We can retrieve rpath or needed dependency with readelf
What do you think ?
For example in some home-made linux distro, there might be readelf and no objdump. (Home-made linux distro do not always relays on package like binutils)

Second remard : RUNPATH or RPATH, could have a list of path, so sed expressions to replace ORIGIN needs g flag

Third remark : ORIGIN value in rpath could be "ORIGIN" or in very rare case I have met "{ORIGIN}"

According to you answer, i will do a second PR

@StudioEtrange StudioEtrange changed the title 3 New options StudioEtrange #1 - Three New options Jul 14, 2016
@ncopa
Copy link
Owner

ncopa commented Jul 15, 2016

@StudioEtrange For binutils, why do you use objdump AND readelf ?

Because the objdump output was cleaner and easier to parse. I assumed that objdump and readelf would be provided from same package, binutils.

I am ok with a patch to only use readelf if:

  1. the patch is clean
  2. we can avoid too much performance penalty

Second remard : RUNPATH or RPATH, could have a list of path, so sed expressions to replace ORIGIN needs g flag

That would been nice to have in separate commit with a clear commit message.

Third remark : ORIGIN value in rpath could be "ORIGIN" or in very rare case I have met "{ORIGIN}"

Does this also happen with objdump?

@ncopa
Copy link
Owner

ncopa commented Jul 15, 2016

I suggest that we go forward with those steps:

  1. close pull requests that have overlapping commits. It is confusing and time consuming to deal with them. (If i'm confused the chance is big that it will either be postponed or ignored)
  2. Fix what is currently broken. In one or more commits. The commit message should clearly explain what the commit fixes. Preferable with a test case so I can reproduce the problem and verify that the fix actually fixes what it should.
  3. Replace objdump with readelf. Explain in the commit message why we do this so if someone in the future wants introduce objdump we can point to a commit message. We should also try avoid spawning too many processes as fork/exec is slow.
  4. Add new features. Each new feature needs its own commit (so if it breaks things i can easily revert) and commit message should explain what the feature is good for. This is so I in the future (or someone else) can read the git log and know exactly why the feature was introduced.

Also, please try avoid add/remove whitespace unless there is good reason to do so. It only slows down the review process.

Thanks!

@StudioEtrange
Copy link
Contributor Author

Third remark : ORIGIN value in rpath could be "ORIGIN" or in very rare case I have met "{ORIGIN}"
Does this also happen with objdump?

Yes, it is hardcoded in binary. it is the value in RUNPATH or RPATH as is "{ORIGIN}"

@StudioEtrange
Copy link
Contributor Author

Sorry, I give up. I do not have enough time to follow your guidelines, and group/edit commit and message.

@StudioEtrange StudioEtrange deleted the upstream branch August 9, 2016 21:16
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.

2 participants