Restore Mac OS X code and buildability #1829

Closed
opened 6 months ago by Moonchild · 21 comments
Owner

Because we'd prefer to not splinter too much across different forks where we can, and in deliberation with @dbsoft who maintains the White Star fork (which is effectively Pale Moon as that is what its usership wants) we should re-unify these efforts and restore the Mac OS X code to UXP.

This will need manual reversal of the efforts in #1751 since we should be working from a LKGS of UXP which unfortunately was considerably after the fork point for it. Dbsoft has offered to do this as part of the mutual effort.

Because we'd prefer to not splinter too much across different forks where we can, and in deliberation with @dbsoft who maintains the White Star fork (which is effectively Pale Moon as that is what its usership wants) we should re-unify these efforts and restore the Mac OS X code to UXP. This will need manual reversal of the efforts in #1751 since we should be working from a LKGS of UXP which unfortunately was considerably after the fork point for it. Dbsoft has offered to do this as part of the mutual effort.
Moonchild added the
OS: Mac OS X
label 6 months ago

Unable to test on MacOSX, but the git revert patch is attached.

The only files that were unable to revert automagically were:

	both modified:   js/xpconnect/src/Sandbox.cpp
	both modified:   toolkit/mozapps/update/updater/updater.cpp
	both modified:   toolkit/xre/nsXREDirProvider.cpp

Reverting this will also fix a bug which was introduced in modules/libmar/verify/cryptox.h which causes modules/libmar/verify/cryptox.h:5: error: unterminated #ifndef when compiling mailcomm ever since #1751 landed.

Unable to test on MacOSX, but the git revert patch is attached. The only files that were unable to revert automagically were: ``` both modified: js/xpconnect/src/Sandbox.cpp both modified: toolkit/mozapps/update/updater/updater.cpp both modified: toolkit/xre/nsXREDirProvider.cpp ``` Reverting this will also fix a bug which was introduced in `modules/libmar/verify/cryptox.h` which causes `modules/libmar/verify/cryptox.h:5: error: unterminated #ifndef` when compiling mailcomm ever since #1751 landed.
Poster
Owner

I really think you should leave this to dbsoft who already indicated he'd work on this, especially if you can't build/test on the target platform.

I really think you should leave this to dbsoft who already indicated he'd work on this, especially if you can't build/test on the target platform.

Agreed. Ultimately I just wanted to get a more recent mailcomm build working and reverting the commit was the easiest way forward. Hopefully the patch will be of some assistance to @dbsoft's task.

Agreed. Ultimately I just wanted to get a more recent mailcomm build working and reverting the commit was the easiest way forward. Hopefully the patch will be of some assistance to @dbsoft's task.
Poster
Owner

I think we're in a good spot to have this work done now. ping @dbsoft

I think we're in a good spot to have this work done now. ping @dbsoft

Ok, need a few clarifications about changes made in these commits.

Mode was change from HTML to XML in a number of the .xul files in 6f707bde95 ... should those changes get reverted or stay?

There are a few changes to de-unify some sources in 1fe9c19305 and maybe a couple other commits that I assume I shouldn't revert?

Ok, need a few clarifications about changes made in these commits. Mode was change from HTML to XML in a number of the .xul files in https://repo.palemoon.org/MoonchildProductions/UXP/commit/6f707bde95dab6998ac204f9ee6c925ee230c740 ... should those changes get reverted or stay? There are a few changes to de-unify some sources in https://repo.palemoon.org/MoonchildProductions/UXP/commit/1fe9c19305dadf2d5bcaa0e589fcd250389dfa8a and maybe a couple other commits that I assume I shouldn't revert?
Collaborator

Mode was change from HTML to XUL in a number of the .xul files in 6f707bde95 ... should those changes get reverted or stay?

Well, I'm thinking that might have just been slipped in naturally while the files were being worked on, but technically XUL is XML and not HTML or SGML, and it definitely isn't Java. So I would think it would be desirable to keep them, unless having those files in XUL mode does cause some kind of build issue on Mac OS in which case it probably would be understandable to revert them.

There are a few changes to de-unify some sources in 1fe9c19305 and maybe a couple other commits that I assume I shouldn't revert?

Seems like it would really depend on whether the browser builds deunified on Mac OS. We never tested Mac, so if it causes a problem there, we might have to discuss reverting that or figuring out how to make it work on Mac.

Basically, one of your primary responsibilities as MacOS maintainer aside from restoring that build to working order will be to point out other, unrelated code changes that break things on Mac since the other developers here cannot test their changes on Mac. I'm in a similar position with SunOS.

The MacOS removal messed up a few ifdefs and therefore has caused some unrelated build failures with Mail applications, though, so I am hoping to test that to see that all is well after you revert the majority of these changes. :)

> Mode was change from HTML to XUL in a number of the .xul files in https://repo.palemoon.org/MoonchildProductions/UXP/commit/6f707bde95dab6998ac204f9ee6c925ee230c740 ... should those changes get reverted or stay? > Well, I'm thinking that might have just been slipped in naturally while the files were being worked on, but technically XUL *is* XML and not HTML or SGML, and it definitely isn't Java. So I would think it would be desirable to keep them, unless having those files in XUL mode does cause some kind of build issue on Mac OS in which case it probably would be understandable to revert them. > There are a few changes to de-unify some sources in https://repo.palemoon.org/MoonchildProductions/UXP/commit/1fe9c19305dadf2d5bcaa0e589fcd250389dfa8a and maybe a couple other commits that I assume I shouldn't revert? > Seems like it would really depend on whether the browser builds deunified on Mac OS. We never tested Mac, so if it causes a problem there, we might have to discuss reverting that or figuring out how to make it work on Mac. Basically, one of your primary responsibilities as MacOS maintainer aside from restoring that build to working order will be to point out other, unrelated code changes that break things on Mac since the other developers here cannot test their changes on Mac. I'm in a similar position with SunOS. The MacOS removal messed up a few ifdefs and therefore has caused some unrelated build failures with Mail applications, though, so I am hoping to test that to see that all is well after you revert the majority of these changes. :)
Poster
Owner

If you're talking about the mode-lines then yeah I changed those bexause XUL should be treated as XML in editors, not HTML. It would be nice to keep those mode lines to what I changed them to while I was in there.

As for de-unification I want to re-visit that later on and as Athenian said we never verified with Mac if there was any Mac specific deprot in the Mozilla code, so the safe way would be to revert the unified building parts as well. Get it working first, then re-visit, so as to not overwhelm people with too many things breaking at once.

If you're talking about the mode-lines then yeah I changed those bexause XUL should be treated as XML in editors, not HTML. It would be nice to keep those mode lines to what I changed them to while I was in there. As for de-unification I want to re-visit that later on and as Athenian said we never verified with Mac if there was any Mac specific deprot in the Mozilla code, so the safe way would be to revert the unified building parts as well. Get it working first, then re-visit, so as to not overwhelm people with too many things breaking at once.

Right, I meant XML, had XUL on the brain.... ok I'll preserve those changes...

Regarding the de-unified source changes, I imported most of your de-unify changes in my own tree when I saw them and didn't run into any dependency problems. However I skipped these changes in their entirety so these specific changes were not tested in my tree either. So you are probably right I should just revert them and revisit later. Thanks!

I'm fortunate to be in the position to test on pretty much any platform, so I'll make sure the code in my branch compiles on pretty much everything before I put in a pull request.

Right, I meant XML, had XUL on the brain.... ok I'll preserve those changes... Regarding the de-unified source changes, I imported most of your de-unify changes in my own tree when I saw them and didn't run into any dependency problems. However I skipped these changes in their entirety so these specific changes were not tested in my tree either. So you are probably right I should just revert them and revisit later. Thanks! I'm fortunate to be in the position to test on pretty much any platform, so I'll make sure the code in my branch compiles on pretty much everything before I put in a pull request.

I've got my preliminary work done... I built on the latest MacOS Monterey and on Ubuntu 22.04 ... a build for Windows is currently running. I had a small error building on MacOS 10.11 El Capitan, but I think I know the problem there... may or may not be a follow up commit coming for that, might be fixable in .mozconfig.

But let me know if there are any glaring errors that you see.

https://repo.palemoon.org/dbsoft/UXP/src/branch/1829

I've got my preliminary work done... I built on the latest MacOS Monterey and on Ubuntu 22.04 ... a build for Windows is currently running. I had a small error building on MacOS 10.11 El Capitan, but I think I know the problem there... may or may not be a follow up commit coming for that, might be fixable in .mozconfig. But let me know if there are any glaring errors that you see. https://repo.palemoon.org/dbsoft/UXP/src/branch/1829
Poster
Owner

I'm not sure if it makes much sense to do a code review on the bulk of it -- that's probably 50k+ changes. If you could indicate parts that didn't revert cleanly to have a glance at for verification, that would help.

I'm not sure if it makes much sense to do a code review on the bulk of it -- that's probably 50k+ changes. If you could indicate parts that didn't revert *cleanly* to have a glance at for verification, that would help.

I should I have kept a log of all the files that had merge conflicts, didn't think about it at the time... I can go back through it and get a list. But essentially all of the conflicts involved about 3 things. #ifdef or other conditional logic involving Android support which was removed in later commits. The removal of telemetry and the removal of xulrunner.

Some files that I can note of being of special interest due to diffs in my Terminal command history:

build/gyp.mozbuild
(the logic was moved into gyp_base.mozbuild)
old-configure.in
dom/html/HTMLInputElement.[cpp|h]
xpcom/io/nsNativeCharsetUtils.cpp
(The unix logic was all removed in a later commit)

Windows successfully built, the issue on El Capitan is using the wrong C++ library, I had to specify it and it works. Not sure why it is different from my fork, which didn't require it being explicitly chosen on that version of MacOS.

I should I have kept a log of all the files that had merge conflicts, didn't think about it at the time... I can go back through it and get a list. But essentially all of the conflicts involved about 3 things. #ifdef or other conditional logic involving Android support which was removed in later commits. The removal of telemetry and the removal of xulrunner. Some files that I can note of being of special interest due to diffs in my Terminal command history: build/gyp.mozbuild (the logic was moved into gyp_base.mozbuild) old-configure.in dom/html/HTMLInputElement.[cpp|h] xpcom/io/nsNativeCharsetUtils.cpp (The unix logic was all removed in a later commit) Windows successfully built, the issue on El Capitan is using the wrong C++ library, I had to specify it and it works. Not sure why it is different from my fork, which didn't require it being explicitly chosen on that version of MacOS.

Ok I went through and made a list of conflicts commit by commit for you to review:

378738aaa9:
build/gyp.mozbuild
gfx/skia/generate_mozbuild.py
gfx/skia/moz.build
xulrunner/stub/moz.build (removed)
93407295fc:
dom/moz.build
dom/geolocation/moz.build
dom/media/systemservices/moz.build
dom/system/moz.build
dom/xbl/builtin/moz.build
3daf711085:
js/xpconnect/src/Sandbox.cpp
toolkit/components/telemetry/histogram-whitelists.json (removed)
toolkit/components/telemetry/Histograms.json (removed)
toolkit/components/url-classifier/nsUrlClassifierUtils.cpp (removed)
xulrunner/app/xulrunner.js (removed)
xulrunner/stub/nsXULStub.cpp (removed)
1a31160dde
xpcom/io/nsNativeCharsetUtils.cpp
xpcom/io/nsNativeCharsetUtils.h
xpcom/threads/ThreadStackHelper.cpp (removed)
xpcom/threads/ThreadStackHelper.h (removed)
9e2a89c71d
js/src/jsmath.cpp
c82b759d56
dom/html/HTMLInputElement.cpp
dom/html/HTMLInputElement.h
768231cf6a
old-configure.in

Two things left if these changes are acceptable, I'm going to put the deunified sources changes back in and do build tests on Mac and if it works recommit those changes.

I am going to diff against my White Star/UXP tree and see if I can figure out what the difference is that is causing this tree to use the wrong C++ library on El Capitan. Might have a follow up commit for that, although it is fixable through a change to .mozconfig.

Ok I went through and made a list of conflicts commit by commit for you to review: https://repo.palemoon.org/dbsoft/UXP/commit/378738aaa9924d0b95e2c57f27cbad2b2e644282: build/gyp.mozbuild gfx/skia/generate_mozbuild.py gfx/skia/moz.build xulrunner/stub/moz.build (removed) https://repo.palemoon.org/dbsoft/UXP/commit/93407295fc1e294855b7943cb00c00d2d4debc02: dom/moz.build dom/geolocation/moz.build dom/media/systemservices/moz.build dom/system/moz.build dom/xbl/builtin/moz.build https://repo.palemoon.org/dbsoft/UXP/commit/3daf711085889bad1bd68651bc4e8790412ae105: js/xpconnect/src/Sandbox.cpp toolkit/components/telemetry/histogram-whitelists.json (removed) toolkit/components/telemetry/Histograms.json (removed) toolkit/components/url-classifier/nsUrlClassifierUtils.cpp (removed) xulrunner/app/xulrunner.js (removed) xulrunner/stub/nsXULStub.cpp (removed) https://repo.palemoon.org/dbsoft/UXP/commit/1a31160dde95477993fb7d3aa1327b2a45f8fca8 xpcom/io/nsNativeCharsetUtils.cpp xpcom/io/nsNativeCharsetUtils.h xpcom/threads/ThreadStackHelper.cpp (removed) xpcom/threads/ThreadStackHelper.h (removed) https://repo.palemoon.org/dbsoft/UXP/commit/9e2a89c71ddf67975da35eb100673f6b5546f292 js/src/jsmath.cpp https://repo.palemoon.org/dbsoft/UXP/commit/c82b759d56454bcd3eac54c484569e1239f7d1cc dom/html/HTMLInputElement.cpp dom/html/HTMLInputElement.h https://repo.palemoon.org/dbsoft/UXP/commit/768231cf6ad1bcd1df02a8f4eecbb2033a8f59b6 old-configure.in Two things left if these changes are acceptable, I'm going to put the deunified sources changes back in and do build tests on Mac and if it works recommit those changes. I am going to diff against my White Star/UXP tree and see if I can figure out what the difference is that is causing this tree to use the wrong C++ library on El Capitan. Might have a follow up commit for that, although it is fixable through a change to .mozconfig.

Still testing but I think I may have discovered the compiler problem.

39f9ab375b

build/moz.configure/toolchain.configure:
if c_compiler.type == 'clang' and target.os == 'Android':
changed to:
if c_compiler.type == 'clang':

Even if this isn't the cause of the problem I am experiencing it is a logic error. The old code only returned empty with clang and Android, now it returns empty for clang on everything.

I am not fluent enough in python to know if I can just remove that conditional section in its entirety.

Update: This was in fact the problem on El Capitan. I tried to put the test back in as it was, but Android is no longer in the list of possible targets, so I just put in OpenBSD to test it and it succeeded. Could someone who knows python better than me show what the correct way to remove this test completely would be?

Still testing but I think I may have discovered the compiler problem. https://repo.palemoon.org/MoonchildProductions/UXP/commit/39f9ab375b2bfd9e46df9695b78870cf1e9cf3c6 build/moz.configure/toolchain.configure: if c_compiler.type == 'clang' and target.os == 'Android': changed to: if c_compiler.type == 'clang': Even if this isn't the cause of the problem I am experiencing it is a logic error. The old code only returned empty with clang and Android, now it returns empty for clang on everything. I am not fluent enough in python to know if I can just remove that conditional section in its entirety. Update: This was in fact the problem on El Capitan. I tried to put the test back in as it was, but Android is no longer in the list of possible targets, so I just put in OpenBSD to test it and it succeeded. Could someone who knows python better than me show what the correct way to remove this test completely would be?
Poster
Owner

if c_compiler.type == ‘clang’ and target.os == ‘Android’:

Yes, that was a mistake. the entire section can go because it's an "and" conditional and should only be triggered on Android. with that now being "always with clang" the result is wrong and would affect all clang compile environments.
If you're not sure about where this definition is used (Mozilla makes python slither around in many coils and knots) then you can just restore the Android conditional and I'll look at it myself later to clean up, just add a comment above it that it can be removed.

EDIT: Oh, sorry, I didn't see your update there. You can just replace the condition with False if the target is no longer present, that will alwasy skip the block.
if c_compiler.type == ‘clang’ and False: should work. Or just if False: for that matter.

> if c_compiler.type == ‘clang’ and target.os == ‘Android’: Yes, that was a mistake. the entire section can go because it's an "and" conditional and should only be triggered on Android. with that now being "always with clang" the result is wrong and would affect all clang compile environments. If you're not sure about where this definition is used (Mozilla makes python slither around in many coils and knots) then you can just restore the Android conditional and I'll look at it myself later to clean up, just add a comment above it that it can be removed. EDIT: Oh, sorry, I didn't see your update there. You can just replace the condition with `False` if the target is no longer present, that will alwasy skip the block. `if c_compiler.type == ‘clang’ and False:` should work. Or just `if False:` for that matter.
Poster
Owner

I looked through the most pertinent files and didn't see any glaring mistakes. If you want to open a PR for this I can merge it in.

I looked through the most pertinent files and didn't see any glaring mistakes. If you want to open a PR for this I can merge it in.

I made one more commmit with the fix for clang and readding the de-unified source changes... it built fine on Mac, doing builds on Linux and Windows now... if they both succeed I'll do the pull request. Thanks!

I made one more commmit with the fix for clang and readding the de-unified source changes... it built fine on Mac, doing builds on Linux and Windows now... if they both succeed I'll do the pull request. Thanks!

So I apparently forked just before you reverted modules/libmar/verify/cryptox.h ... so the pull request says they are conflicting, although the files seem to be identical. How do I resolve the conflict in the pull request?

So I apparently forked just before you reverted modules/libmar/verify/cryptox.h ... so the pull request says they are conflicting, although the files seem to be identical. How do I resolve the conflict in the pull request?
Poster
Owner

merge UXP master into your branch to resolve the conflict locally in your branch, then the updated branch should be fine as the pull request

merge UXP master into your branch to resolve the conflict locally in your branch, then the updated branch should be fine as the pull request

Ok done, seems a bit overkill for a single identical file but... whatever works. :)

Ok done, seems a bit overkill for a single identical file but... whatever works. :)
Poster
Owner

Ok done, seems a bit overkill for a single identical file but... whatever works. :)

It's the standard way to bring a branch up to date with the root. there are other ways to go about it but it wouldn't be consistent (and probably more work in the end, as well :P )

> Ok done, seems a bit overkill for a single identical file but... whatever works. :) It's the standard way to bring a branch up to date with the root. there are other ways to go about it but it wouldn't be consistent (and probably more work in the end, as well :P )
Moonchild added this to the 31.0.0 milestone 5 months ago
Moonchild added the
Done
label 5 months ago
Poster
Owner

Assuming this is done, closing.

Assuming this is done, closing.
Moonchild closed this issue 5 months ago
Sign in to join this conversation.
No Milestone
No Assignees
4 Participants
Notifications
Due Date

No due date set.

Dependencies

No dependencies set.

Reference: MoonchildProductions/UXP#1829
Loading…
There is no content yet.