2070532 - Install function "GetPostUpdateTarget" not referenced

Status: ASSIGNED

(Firefox :: Installer, defect, P3)

Description

[Antonio Borondo [:aborondo] (he/him)]

As a developer, I want to eliminate warning 6010 when packaging Firefox so that ./mach package completes without warnings:

warning 6010: install function "GetPostUpdateTarget" not referenced - zeroing code (3144-3171) out

Comment 1

[BugBot [:suhaib / :marco]]

Set release status flags based on info from the regressing bug 2030812

:dmcintosh, since you are the author of the regressor, bug 2030812, could you take a look?

For more information, please visit BugBot documentation.

Comment 2

[Hackbot [:hackbot]]

postupdate_helper.nsh defines GetPostUpdateTarget, whose only non-test caller is the PostUpdate macro in uninstaller_helpers.nsh, a file setup.exe never includes, so installer.nsi still compiles the function through shared.nsh and makensis emits 6010. The definition came from bug 2030812 Part 2 (5266c2e18598); helper.exe references it as before, so post-update behavior is not affected.

Fix: Guard the definition the way the same problem is already handled at shared.nsh: add a define (say NEED_POSTUPDATE_TARGET) in uninstaller.nsi and test_stub.nsi, and wrap Function GetPostUpdateTarget in an !ifdef on it so only helper.exe and test_stub_installer.exe build it.

Suggested severity: S4. Build-time warning only, with no change to what the shipped installer or the post-update helper does for a user.

---

If you'd like to provide feedback on this comment, please use the 👍 or 👎 reaction.
If you want to categorize your feedback you can add one of the following tags: ai-triage-wrong-file, ai-triage-wrong-cause, ai-triage-hallucination, ai-triage-out-of-scope, ai-triage-wrong-fix, ai-triage-shallow-fix.

Comment 3

[Duncan McIntosh [:dmcintosh]]

Right, I don't remember if I expected that or not. I think this was a conflict between two places:

• uninstaller_helper.nsh wouldn't have this warning, but (if I recall) it wouldn't work in tests, and I wanted coverage for this.
• postupdate_helper.nsh has this warning, but it does work in tests.

I feel like splitting this out into yet another file isn't a good move, but I'm not sure what is a good move. Maybe it could be wrapped with !ifdef UNINSTALLER_HELPERS_H or !ifdef UNINSTALLER_CODE, paired with !define UNINSTALLER_CODE in test_stub.nsh and uninstaller.nsi? [looks like that matches the robot suggestion.]

The classical approach is a macro like those in toolkit/mozapps/installer/windows/nsis/common.nsh, but from what I can tell the trend was (subconsciously?) moving away from that, it is really verbose and it can mask actually-unused functions.

I could write a patch if desired, but it might be easier for you to do so since I don't have a local Win32 build setup and I'm not sure what approach you'd want to go for, let me know what's best. I could review (r?aurora) if desired, still am in browser-installer-reviewers >:)

(As an aside, NSIS looks like it supports a /WX flag to treat warnings as errors. Maybe we should start using that so this gets caught earlier?)

Comment 4

[BugBot [:suhaib / :marco]]

Set release status flags based on info from the regressing bug 2030812

Comment 5

[Stephen Thompson [:sthompson]]

I'm guessing wontfix for 157 if there isn't a patch in progress already

Comment 6

[BugBot [:suhaib / :marco]]

Set release status flags based on info from the regressing bug 2030812

Comment 7

[Stephen A Pohl [:spohl]]

Attachment: Bug 2070532: Limit `GetPostUpdateTarget` to helper and test builds. r?#browser-installer-reviewers!,yjuglaret!

Comment 8

[Mauro Ferreiro [:mferreiro] (he/him)]

(In reply to Duncan McIntosh [:dmcintosh] from comment #3)

Right, I don't remember if I expected that or not. I think this was a conflict between two places:

• uninstaller_helper.nsh wouldn't have this warning, but (if I recall) it wouldn't work in tests, and I wanted coverage for this.
• postupdate_helper.nsh has this warning, but it does work in tests.

I feel like splitting this out into yet another file isn't a good move, but I'm not sure what is a good move. Maybe it could be wrapped with !ifdef UNINSTALLER_HELPERS_H or !ifdef UNINSTALLER_CODE, paired with !define UNINSTALLER_CODE in test_stub.nsh and uninstaller.nsi? [looks like that matches the robot suggestion.]

The classical approach is a macro like those in toolkit/mozapps/installer/windows/nsis/common.nsh, but from what I can tell the trend was (subconsciously?) moving away from that, it is really verbose and it can mask actually-unused functions.

I could write a patch if desired, but it might be easier for you to do so since I don't have a local Win32 build setup and I'm not sure what approach you'd want to go for, let me know what's best. I could review (r?aurora) if desired, still am in browser-installer-reviewers >:)

(As an aside, NSIS looks like it supports a /WX flag to treat warnings as errors. Maybe we should start using that so this gets caught earlier?)