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

Implement resolve-system-dependencies in C++ #1030

Merged
merged 11 commits into from
Aug 31, 2016
Merged

Conversation

pikajude
Copy link
Contributor

Instead of serializing a hashmap to a file as Perl does (which would require an external library dependency), this script stores the dependency list for each lib separately in a cache directory. This should be faster than the old implementation because no resolving is done after the first time the script runs.

Accepting code review as my C++ is not very clean.

@domenkozar domenkozar added this to the perl-to-c++ milestone Aug 15, 2016
static auto cacheDir = Path{};

Path resolveCacheFile(const Path & lib) {
Path lib2 = Path(lib);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of using a const reference parameter and doing explicit copying, you could directly accept the lib argument via value.

@copumpkin
Copy link
Member

At the highest level, I'd suggest using mmap because it saves you from doing 90% of the nasty seeking/reading/sizeof-ing. You map the object into memory and then the various structs already point at the correct data. That's how dyld does it, and I think it'll make your life a lot simpler.

@copumpkin
Copy link
Member

(not that I'd block merging this because you didn't use mmap, but I think it'd be a bit more pleasant)

dylc.dylib.name.offset - (sizeof(uint32_t) * 2) + pos,
SEEK_SET);
fread(dylib_name, sizeof(char), cmd.cmdsize, obj_file);
fseek(obj_file, pos, SEEK_SET);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This lacks error checking. I would use C++ <iostream>, or read the whole file into memory using readFile() for simplicity.

@domenkozar
Copy link
Member

@pikajude does this address #941?

@domenkozar
Copy link
Member

Also relevant is #786

@edolstra edolstra mentioned this pull request Aug 16, 2016
12 tasks
@pikajude
Copy link
Contributor Author

pikajude commented Aug 16, 2016

@domenkozar #941 will no longer apply because otool is not used or even required in this rewrite (the mach-o files are directly read by the script instead). #786 looks to be a fundamental problem with pre-build hooks in general, and as such will require more investigation.

@edolstra @copumpkin I have rewritten the script to use mmap, meaning there should be a little less fiddling around with FILE*s.

@edolstra edolstra merged commit aa1ea0d into NixOS:master Aug 31, 2016
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.

5 participants