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

CMake building and Initial REL(A) support #37

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

Conversation

deadly-platypus
Copy link

I needed to at least be able to get relocation entries for my work, so I quickly added that by adding two new methods of the section class, get_rels and get_relas. Both return a std::vector of a new struct Elf_Rel or struct Elf_Rela instances. Additionally, I added CMake support, including the creation of the to_string.cc files in both the dwarf and elf directories. This PR only builds the libraries in those directories, with targets for the shared and static libraries. So the tests or examples still need work.

Now comes the bad part. I had my IDE reformat all the code, which created a large diff. You can take it as is, or let me know what styling you use, and I can update this PR. Or, you know, you could just say no to it all.

@nicolas17
Copy link

I think CMake, reformatting code, and adding rels should be separate pull requests.

@deadly-platypus
Copy link
Author

Any suggestion as to how to do that?

@banshee banshee mentioned this pull request Jun 26, 2020
@aclements
Copy link
Owner

Hi @deadly-platypus . Unfortunately, with all of the code reformatting I can't really review or submit this. Please just follow the style of the existing code, which should be pretty consistent (though I didn't run a formatting tool, so I'm sure there are minor inconsistencies).

I'm curious about the CMake support you added, since this is the second PR to propose that. What's the benefit of adding CMake support? Make is certainly not the best thing in the world, but the existing makefiles work and are pretty simple. (I've never used CMake myself, so perhaps the answer is obvious to someone more familiar with it.)

@deadly-platypus
Copy link
Author

I totally understand rejecting this PR for the formatting. I'll try to fix that soon.

Cmake is used to generate a build system, be it make files, Visual Studio... Whatever they use...., Ninja files, or others. It thus makes it easy to integrate other repos into existing projects as git submodules. So to add this repo as a dependency for my project, I as it as a submodule, and then add two lines to the CMakeLists.txt file. One line is to tell CMake about the directory, and one line to add the build target as a dependency. CMake then ensures that libelfin is built before my code, and I don't need to know how your code is built because CMake will generate the correct build system for everything. IMO it's a pretty slick system for better cross platform integration and usability.

@madebr
Copy link

madebr commented Oct 23, 2020

@aclements @deadly-platypus
Are you (=the authors) open to accepting a cmake pull request?
I'm asking because atm, there is a request at conan-center-index for adding a libelfin recipe which includes a cmake script.

If you're open to adding a cmake script upstream, then we don't have to maintain one ourselves.
(you can always tag/ask us if you happen to have problems/questions once merged)

axel-capodaglio added a commit to axel-capodaglio/libelfin that referenced this pull request Mar 14, 2024
merge manuale senza riformattazioni da aclements#37, fino a commit (incluso)
"changing elf lib domain model, and adding some stubs "
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.

4 participants