Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

fix: Add more definitions to built .a #3

Merged
merged 1 commit into from
May 7, 2024
Merged

Conversation

aran
Copy link
Contributor

@aran aran commented Apr 17, 2024

Add more C files as dependencies, along with several more defines, to define more symbols in the built result.

Tested by building and running a rust binary that uses diesel with this as the dependency, on a mac, with

  • no --platforms
  • a linux-x86_64 platform
  • a linux-arm64 platform

Fixes #2.

@dmeijboom
Copy link
Member

@aran Sorry for the delay. We tested your changes on a couple of internal repositories and it seems to work just fine. Thanks!

Copy link
Member

@dmeijboom dmeijboom left a comment

Choose a reason for hiding this comment

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

The code LGTM but at brainhive all of the commits have to be signed. Can you do that? For more information: https://docs.github.com/en/authentication/managing-commit-signature-verification/signing-commits

@aran
Copy link
Contributor Author

aran commented May 7, 2024

OK, fixed up that commit verification issue.

I also removed the MODULE.bazel.lock change, not sure if you wanted that included. If so, I figure you can decide that and commit it, since it's not strictly related to this.

With respect to the changes in this PR, it's been working for me without issue so far, but just want to be clear it is a bit concerning to me on several aspects:

  1. I don't understand how you had things working without something like it, which makes me feel like I don't understand something about the overall build environment
  2. I don't understand the implications of blanking out getpeereid, or why the postgres source doesn't work out of the box with the configured options
  3. It seems reasonable that FRONTEND is right but don't understand the implications of the STRERROR defines.

Copy link
Member

@dmeijboom dmeijboom left a comment

Choose a reason for hiding this comment

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

LGTM

@dmeijboom dmeijboom merged commit ebdeb0c into brainhivenl:main May 7, 2024
@dmeijboom
Copy link
Member

OK, fixed up that commit verification issue.

I also removed the MODULE.bazel.lock change, not sure if you wanted that included. If so, I figure you can decide that and commit it, since it's not strictly related to this.

Check! Let me know if it works for you on the main branch (without the MODULE.bazel.lock change). If not I'll add it anyways.

With respect to the changes in this PR, it's been working for me without issue so far, but just want to be clear it is a bit concerning to me on several aspects:

1. I don't understand how you had things working without something like it, which makes me feel like I don't understand something about the overall build environment

I can say with confidence that I have a very poor understanding of how all of this works in the first place. It has been a matter of making changes, testing, verifying, etc. I still don't get why the original version worked (it might be due to some magic happening in pq-sys or with our toolchain not being fully hermetic). But it works and passes all of our tests so I guess it should be fine :-)

2. I don't understand the implications of blanking out `getpeereid`, or why the postgres source doesn't work out of the box with the configured options
3. It seems reasonable that `FRONTEND` is right but don't understand the implications of the `STRERROR` defines.

Me neither. If I have some time in the upcoming weeks I'll look further into this. I also wanted to add support for more architectures.

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

Successfully merging this pull request may close these issues.

Built libpq.a is missing many symbols
2 participants