1896632 - Perma Windows MinGW [tier 2] /builds/worker/checkouts/gecko/widget/windows/JumpListBuilder.cpp:708:48: error: use of undeclared identifier 'PKEY_Link_Arguments'

Status: RESOLVED FIXED

(Core :: Widget: Win32, defect)

Description

[Yannis Juglaret [:yannis] | PTO back Oct 16]

Mingw builds are currently broken (see bug 1895428) but bug 1892070 seems to have introduced a new cause for mingw build failure on top of the one that we had already identified. [:mconley], could you take a look?

Example job: here

/builds/worker/checkouts/gecko/widget/windows/JumpListBuilder.cpp:708:48: error: use of undeclared identifier 'PKEY_Link_Arguments'

If we think it's useful, the long-term fix is to update propkey.h upstream in mingw-64 based on Microsoft's version of the file (located at C:\Program Files (x86)\Windows Kits\10\Include\10.0.22621.0\um\propkey.h on my machine). Let me know if you'd like to try it on your own, otherwise I could do that part.

The short-term fix is to use a #ifdef __MINGW32__. We can use it to comment out the faulty code in mingw builds assuming it is OK to not have this feature in Tor. Or we can use it as a guard condition to manually define the missing constant. I think here the constant can be defined by adding this line of C++ (based on the Microsoft version of propkey.h):

DEFINE_PROPERTYKEY(PKEY_Link_Arguments, 0x436F2667, 0x14E2, 0x4FEB, 0xB3, 0x0A, 0x14, 0x6C, 0x53, 0xB5, 0xB6, 0x74, 100);

It's up to you to choose what's most appropriate here. Then when we update to a more recent version of mingw-w64 we will be able to remove that #ifdef if we went for the long-term fix. But please make sure to implement a short-term fix anyway so that we can have the mingw builds back.

Comment 1

[Mike Conley (:mconley) (:⚙️)]

We use PKEY_Link_Arguments in nsWindowsShellService, and it looks like it solves this problem using this #ifdef: https://searchfox.org/mozilla-central/rev/b3f40fd7c4671537ed29a232e76c962977650045/browser/components/shell/nsWindowsShellService.cpp#50-55

Would it be sufficient to do the same?

Comment 2

[Yannis Juglaret [:yannis] | PTO back Oct 16]

(In reply to Mike Conley (:mconley) (:⚙️) from comment #1)

We use PKEY_Link_Arguments in nsWindowsShellService, and it looks like it solves this problem using this #ifdef: https://searchfox.org/mozilla-central/rev/b3f40fd7c4671537ed29a232e76c962977650045/browser/components/shell/nsWindowsShellService.cpp#50-55

That part is for PropVariantToString. The use of PKEY_Link_Arguments in nsWindowsShellService.cpp is commented out for mingw builds as follows:

NS_IMETHODIMP
nsWindowsShellService::GetTaskbarTabPins(nsTArray<nsString>& aShortcutPaths) {
#ifdef __MINGW32__
  return NS_ERROR_NOT_IMPLEMENTED;
#else
  // ...
    // Get the PKEY_Link_Arguments property
    hr = pps->GetValue(PKEY_Link_Arguments, &propVar);
  // ...
#endif
}

We can either add the definition like in comment 0, guarded by a #ifdef __MINGW32__ block, or comment out the code for mingw builds like in the example just above.

Comment 3

[BugBot [:suhaib / :marco/ :calixte]]

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

Comment 4

[Mike Conley (:mconley) (:⚙️)]

Attachment: Bug 1896632 - Manually define PKEY_Link_Arguments in JumpListBuilder for MINGW32 builds. r?yjuglaret

Comment 5

[Mike Conley (:mconley) (:⚙️)]

Try push: https://treeherder.mozilla.org/jobs?repo=try&revision=305e5af4af7d5cfa8dea64b9d7e88046b4868b36

Comment 6

[Mike Conley (:mconley) (:⚙️)]

Thanks for the quick review! Is this the sort of thing we'll want to uplift to Beta for downstream projects like Tor Browser?

Comment 7

[Yannis Juglaret [:yannis] | PTO back Oct 16]

That is a very good remark. According to this document, Tor is built from the ESR branch, and installing the latest version of Tor confirms that as it shows 115.11.0esr in about:support, so I think we should be fine. Edit: Note that downloading Nightly Tor Browser builds also yields 115.11.0esr.

Comment 8

[Yannis Juglaret [:yannis] | PTO back Oct 16]

Redirecting to the release owner [:pascalc], would it be useful to fix mingw builds on the 127 branch in general? (despite comment 7)

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e5c83d7e6c8b
Manually define PKEY_Link_Arguments in JumpListBuilder for MINGW32 builds. r=yjuglaret,win-reviewers

Comment 10

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/e5c83d7e6c8b

Comment 11

[Pascal Chevrel:pascalc]

Feel free top request an uplift to beta.

Comment 12

[BugBot [:suhaib / :marco/ :calixte]]

The patch landed in nightly and beta is affected.
:mconley, is this bug important enough to require an uplift?

• If yes, please nominate the patch for beta approval.
• If no, please set status-firefox127 to wontfix.

For more information, please visit BugBot documentation.

Comment 13

[Mike Conley (:mconley) (:⚙️)]

Attachment: Bug 1896632 - Manually define PKEY_Link_Arguments in JumpListBuilder for MINGW32 builds.

Original Revision: https://phabricator.services.mozilla.com/D210315

Comment 14

[Phabricator Automation]

beta Uplift Approval Request

• User impact if declined: Users who build Firefox using MINGW32 will find that they cannot.
• Code covered by automated testing: yes
• Fix verified in Nightly: yes
• Needs manual QE test: no
• Steps to reproduce for manual QE testing: N/A
• Risk associated with taking this patch: Low
• Explanation of risk level: Very little - this fixes a build issue for the MINGW32 configuration.
• String changes made/needed: None.
• Is Android affected?: no

Comment 15

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-beta/rev/05fd1a91e655