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

Add --build-id to the linker flags #159

Merged
merged 1 commit into from
Jan 7, 2025
Merged

Add --build-id to the linker flags #159

merged 1 commit into from
Jan 7, 2025

Conversation

bendk
Copy link
Contributor

@bendk bendk commented Dec 31, 2024

See https://bugzilla.mozilla.org/show_bug.cgi?id=1937916 for the reasoning here. I tested this with a Maven local publish and it worked for me.

This could also be behind an option, either one specifically for build-id or a general one like EXTRA_LINKER_ARGS. It seems like build-id is a good default though so I didn't implement it that way on the first pass.

@bendk bendk requested a review from ncalexan January 3, 2025 15:18
@ncalexan
Copy link
Member

ncalexan commented Jan 6, 2025

I have 2 questions:

  1. I guess Rust's linking stage doesn't include a build ID by default? If it doesn't, then I feel like we should put this behind a flag, so that we don't surprise consumers. Or is it that by specifying -Wl at all, we're turning of the Rust default, which would include the build ID?
  2. Further, this is a pretty visible change in behaviour. Do you agree that if we enable by default, that means bumping more than the patch version?

@mstange
Copy link

mstange commented Jan 6, 2025

  1. I guess Rust's linking stage doesn't include a build ID by default? If it doesn't, then I feel like we should put this behind a flag, so that we don't surprise consumers. Or is it that by specifying -Wl at all, we're turning of the Rust default, which would include the build ID?

I haven't tested it myself, but here's a discussion from 2021 which states that Rust by default does not include a build ID: https://internals.rust-lang.org/t/emitting-build-id-when-linking-elf-binaries/15349

@ncalexan
Copy link
Member

ncalexan commented Jan 7, 2025

  1. I guess Rust's linking stage doesn't include a build ID by default? If it doesn't, then I feel like we should put this behind a flag, so that we don't surprise consumers. Or is it that by specifying -Wl at all, we're turning of the Rust default, which would include the build ID?

I haven't tested it myself, but here's a discussion from 2021 which states that Rust by default does not include a build ID: https://internals.rust-lang.org/t/emitting-build-id-when-linking-elf-binaries/15349

OK. I'd be happier making this opt-in. @bendk?

@bendk bendk force-pushed the build-id-linker-flag branch from 61a6d5d to e0a4141 Compare January 7, 2025 17:57
@bendk
Copy link
Contributor Author

bendk commented Jan 7, 2025

Sounds good, I just updated the PR to make this optional.

One issue I noticed is that there seems to be an issue with stale objects. In order for the change to take effect I needed to run cargo clean, then ./gradlew assembleDebug twice. Any idea what's going on here and how I can fix it?

### generateBuildId

Generate a build-id for the shared library during the link phase.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Any suggestions on how to better describe this? I think a link would be great, but when I did a quick search I didn't find a good page to send people to.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm also open to changing the config name.

Copy link
Member

Choose a reason for hiding this comment

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

I don't see a better link either; maybe https://linux.die.net/man/1/ld? I see some options, but that can be for the next person to monkey with.

Copy link
Member

@ncalexan ncalexan left a comment

Choose a reason for hiding this comment

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

Fine by me. Do you know how to cut a release? There are docs in the repo, give it a try?

### generateBuildId

Generate a build-id for the shared library during the link phase.

Copy link
Member

Choose a reason for hiding this comment

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

I don't see a better link either; maybe https://linux.die.net/man/1/ld? I see some options, but that can be for the next person to monkey with.

@bendk
Copy link
Contributor Author

bendk commented Jan 7, 2025

Fine by me. Do you know how to cut a release? There are docs in the repo, give it a try?

Sounds good, let's see what happens.

@bendk bendk merged commit 9934bdf into master Jan 7, 2025
28 checks passed
bendk added a commit to bendk/application-services that referenced this pull request Jan 13, 2025
Upgrade rust-android-gradle and set the `generateBuildId` flag..

See mozilla/rust-android-gradle#159 for details.
bendk added a commit to bendk/application-services that referenced this pull request Jan 13, 2025
Upgrade rust-android-gradle and set the `generateBuildId` flag..

See mozilla/rust-android-gradle#159 for details.
bendk added a commit to bendk/application-services that referenced this pull request Jan 13, 2025
Upgrade rust-android-gradle and set the `generateBuildId` flag.

See mozilla/rust-android-gradle#159 for details.
github-merge-queue bot pushed a commit to mozilla/application-services that referenced this pull request Jan 13, 2025
Upgrade rust-android-gradle and set the `generateBuildId` flag.

See mozilla/rust-android-gradle#159 for details.
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