Initial work on Mac patching #12

colincornaby · 2023-10-15T07:22:18Z

This is kind of ugly but seems to work.

I'm using tgz for the Mac app bundle in since it's a directory. That should be ok in since the Mac client is the only one that will need to look at these manifests - but it would require the client now handle the tar format.

I don't know if there is a better way that doesn't involve tar'ing. I could store as separate files but the logic starts to get more complicated at knowing what to keep and what to delete if we want to get rid of the existing bundle entirely.

colincornaby · 2023-10-15T17:52:51Z

I'm looking through to see if an alternate compression mechanism for directories would be better. Plasma doesn't include a TAR library but it does include a lot of other things. There might be an alternative that works better for Plasma.

urumanifest/commit.py

urumanifest/github.py

Hoikas · 2023-10-15T20:12:10Z

urumanifest/github.py

    with archive.open(info, "r") as infile:
-        with output_path.joinpath(Path(info.filename).name).open("wb") as outfile:
+        full_path = output_path.joinpath(output_subpath)
+        Path(os.path.dirname(full_path)).mkdir(parents=True, exist_ok=True)


What is going on here? This looks like it's making a new directory in the CWD...

It's unpacking the app bundle into the gather directory - but it needs to create any missing folders in the unpack path along the way.

urumanifest/github.py

urumanifest/commit.py

colincornaby · 2023-10-22T07:51:22Z

@Hoikas - I haven't started on matching client side changes yet. I think I'd like feedback on the compression mechanism first.

This PR uses a tarfile in since it's usually complementary for gzip. But that's probably going to require a new library be used in Plasma if our dependencies aren't pulling in a tar library already.

We could also open things up to a wider range of archive formats.

Or I can back off and try to get things working single file. Each file in the bundle gets zipped separately and the flag activates some smarts about treating the parent directory as a single unit. We'd lose some compression by not bundling all the files together but that would be the most compatible with the existing approach.

Hoikas

The only concern I have right now is that we're going to be vulnerable to the order that files are hashed inside the tarachive. Hopefully, that won't come back to bite us... I don't think we currently have a tar library in Plasma. I am unfamiliar with the details of the format, so I can't offer any guidance there.

urumanifest/github.py

urumanifest/commit.py

colincornaby · 2023-10-23T04:20:50Z

The only concern I have right now is that we're going to be vulnerable to the order that files are hashed inside the tarachive. Hopefully, that won't come back to bite us... I don't think we currently have a tar library in Plasma. I am unfamiliar with the details of the format, so I can't offer any guidance there.

(Still working through the other feedback.)

Order that files are MD5'd is a risk. We'll have to make sure all ends are sorting the file the same way.

Tar is a Unix archive format usually used with gzip. Strangely - libtar does not work on Windows. But - libarchive does and libarchive can deal with tar archives. But libarchive also supports a wide array of formats so if we wanted to pick something a bit newer we could.

For macOS specifically - libarchive is also included with the system. Although nothing is stopping us from statically linking on macOS - as we would have to do for the other platforms anyway.

urumanifest/commit.py

urumanifest/github.py

urumanifest/commit.py

colincornaby · 2024-06-02T03:43:16Z

urumanifest/commit.py

@@ -46,6 +54,11 @@ def _hash_asset(args: Tuple[Path, Path]) -> Tuple[Path, str, int]:
    server_path, source_path = args
    # One day, we will not use such a vulnerable hashing algo...
    h = hashlib.md5()
+    if source_path.is_dir():


Something I don't like here: The flag that this is an app bundle seems to be encoded later and isn't available yet. So I'm checking to see if the asset is a directory which is kind of gross. Ideally I'd only be checking the bitmask for the bundle property.

colincornaby · 2024-06-30T19:39:13Z

Going to promote this to ready to review.

The only odd output that I think is present here is that we archive to top level of the bundle instead of the bundle itself. I.E. if you have plClient.app/Contents, the top level of the archive will have Contents and not plClient.app. Even though it's odd - I still think it's proper. The manifest contains the plClient.app destination name - and not encoding that level into the archive avoids duplication between the manifest and the archive, and avoids possible naming conflicts and corner cases. I.E. if the manifest said the name was UruExplorer.app but the manifest said plClient.app - thats a corner case I don't want to deal with.

colincornaby · 2024-07-01T04:43:05Z

The bundle flag is not properly being carried through the script - I'll dig in and figure out why it isn't landing in the final manifest.

urumanifest/commit.py

Hoikas · 2024-07-02T02:22:06Z

urumanifest/dependencies.py

@@ -191,6 +191,7 @@ def track_client_dependency(client_path: Path, server_path: Path):

    def track_manifest_dependency(client_path: Path, server_path: Path, category: str, manifest_names):
        flags = ManifestFlags.installer if category in gather_installers else 0
+        flags |= ManifestFlags.bundle if client_path.suffix == ".app" else 0


It would be nice to have the bundle extension in constants.py instead of as a magic string at the very minimum. On the other hand, the rest of the script (i.e. the Windows support) doesn't try to use magic strings in the filename to identify what flags need to be applied. Instead, we use the the keys from the control json files to assign a category. Then, we apply flags from the category here. So, what we should really be doing is have a dict in the control json to represent the new mac bundle. github.py will need to be updated to generate that part of the fake control json.

I can take a look at that.

Looks like this is still outstanding?

Hoikas · 2024-07-02T02:30:16Z

urumanifest/github.py

+                client_folder_name = "client"
+                if len(member_path.parts) >=  2 and member_path.parts[0] == client_folder_name:
+                    is_mac_app_bundle = member_path.parts[1].endswith(".app")
+                    if not i.is_dir() and len(member_path.parts) ==  2:
+                        yield ArtifactInfo(path=member_path.name, zipinfo=i, name=member_path.name)
+                    elif is_mac_app_bundle and not i.is_dir():
+                        # remove "client/"
+                        path = member_path.relative_to(client_folder_name)
+                        yield ArtifactInfo(path=path, zipinfo=i, name=member_path.parts[1])


What is going on here?

This is constructing paths to the bundle, but not the files in the bundle. For example:
client/plClient.app/Contents/Info.plist
Should only be added to the control json as:
plClient.app

Hoikas · 2024-07-02T02:31:05Z

urumanifest/constants.py

+    "macInternal": _manifests("MacThinInternal", None, "MacInternal"),
+    "macExternal": _manifests("MacThinExternal", None, "MacExternal"),


Are these legacy manifests?

macInternal and macExternal are current. Should they be declared as something else?

They should probably be listed before the comment that says legacy manifests follow.

Hoikas · 2024-07-02T02:31:26Z

urumanifest/constants.py

+    "macexternal": _directorytuple("", "client/mac/external"),
+    "macinternal": _directorytuple("", "client/mac/internal"),


The filename case doesn't match above. Also, just to make sure - we are assuming that mac clients are universal binaries?

Yes, thats the assumption. It wouldn't really make sense to have it be a universal binary locally and then a processor specific binary.

That said - that will make things messy when Apple eventually gets around to cutting Intel support from their tools. But that eventuality could be dealt with like any other platform we discontinue support for.

Note that currently CI is not generating universal binaries (because of vcpkg limitations). In theory we could figure out a way to lipo them together into a universal binary when we attach them to the published release.

Note to self: the gather_lut keys are expected to be all lowercase.

dpogue · 2024-07-02T03:35:04Z

urumanifest/constants.py

+    "macexternal": _directorytuple("", "client/mac/external"),
+    "macinternal": _directorytuple("", "client/mac/internal"),


Note that currently CI is not generating universal binaries (because of vcpkg limitations). In theory we could figure out a way to lipo them together into a universal binary when we attach them to the published release.

dpogue · 2024-07-02T03:35:41Z

urumanifest/constants.py

+    "plasma-macos-x64-internal-release": "macInternal",
+    "plasma-macos-x64-external-release": "macExternal",


Just noting that these are Intel x86_64 binaries (which is probably the best option for the moment, but they are not universal because they do not include arm64)

Yeah - that still needs to be done on the CI side. For now, I'm kind of assuming whatever the platforms should be is what is provided by CI.

colincornaby · 2024-07-02T05:03:19Z

I have a structure that I'm working on to separate the Mac bundle - but I don't know if it really fits with how things are supposed to work.

{
  "macExternal": [
    "banner.png",
    "[email protected]",
    "resource.dat"
  ],
  "macBundle": [
    "UruExplorer.app"
  ]
}

This would de-associated the macBundle with macExternal. But I don't think a child map inside macExternal is supported?

Hoikas · 2024-07-02T14:16:51Z

But I don't think a child map inside macExternal is supported?

No, I don't think any of the code is equipped to handle dictionaries inside of dictionaries.

You may want to separate macBundle into macBundleExternal and macBundleInternal to allow shipping down both internal and external release apps.

colincornaby · 2024-07-03T03:30:24Z

@Hoikas - That was going to be my next question. My assumption is somewhere deeper in the code I need to take those keys and pair them with a destination.

colincornaby · 2024-07-07T04:21:49Z

I have an initial version in that uses a custom Gather key for the Mac bundle. However - deeper sections of code are still doing a folder check.

Hoikas

I still need to do a complete rereview, but this is what I see for the two new commits.

urumanifest/github.py

urumanifest/commit.py

Hoikas · 2024-07-14T19:03:49Z

urumanifest/constants.py

+    "macexternal": _directorytuple("", "client/mac/external"),
+    "macinternal": _directorytuple("", "client/mac/internal"),


Note to self: the gather_lut keys are expected to be all lowercase.

Hoikas · 2024-07-14T19:04:43Z

urumanifest/dependencies.py

@@ -191,6 +191,7 @@ def track_client_dependency(client_path: Path, server_path: Path):

    def track_manifest_dependency(client_path: Path, server_path: Path, category: str, manifest_names):
        flags = ManifestFlags.installer if category in gather_installers else 0
+        flags |= ManifestFlags.bundle if client_path.suffix == ".app" else 0


Looks like this is still outstanding?

urumanifest/github.py

Co-authored-by: Adam Johnson <[email protected]>

…h into the bundle. Not sure why this code was written this way, should have never worked…

Client path does not include trailing slash. Checking for a .app suffix instead.

Co-authored-by: Adam Johnson <[email protected]>

Hoikas reviewed Oct 15, 2023

View reviewed changes

colincornaby force-pushed the mac-patch branch 3 times, most recently from 1b3049b to ac9fa72 Compare October 16, 2023 03:43

Hoikas reviewed Oct 22, 2023

View reviewed changes

colincornaby mentioned this pull request Nov 19, 2023

Self patching Mac application H-uru/Plasma#1528

Merged

Hoikas reviewed May 29, 2024

View reviewed changes

urumanifest/commit.py Outdated Show resolved Hide resolved

urumanifest/commit.py Outdated Show resolved Hide resolved

urumanifest/github.py Outdated Show resolved Hide resolved

colincornaby commented Jun 2, 2024

View reviewed changes

urumanifest/commit.py Show resolved Hide resolved

colincornaby commented Jun 2, 2024

View reviewed changes

colincornaby marked this pull request as ready for review June 30, 2024 19:39

Hoikas reviewed Jul 2, 2024

View reviewed changes

dpogue reviewed Jul 2, 2024

View reviewed changes

Hoikas reviewed Jul 10, 2024

View reviewed changes

urumanifest/github.py Outdated Show resolved Hide resolved

urumanifest/commit.py Outdated Show resolved Hide resolved

Hoikas reviewed Jul 14, 2024

View reviewed changes

colincornaby and others added 7 commits July 17, 2024 23:42

Initial work on Mac patching

a5a29e2

Apply suggestions from code review

8439911

Co-authored-by: Adam Johnson <[email protected]>

Update urumanifest/github.py

3067891

Co-authored-by: Adam Johnson <[email protected]>

OS is needed for pathname manipulation

57e2584

Removing magic number in bundle path setup

261367d

Switching to rglob

2f250cd

Converting to named tuple

5bee7f0

colincornaby and others added 15 commits July 17, 2024 23:42

Feedback from code review

9d2fc93

Finishing implementing code review feedback

392222a

Syncing with latest Mac self patching changes

92ed098

Apply suggestions from code review

620bb5a

Co-authored-by: Adam Johnson <[email protected]>

Fixing the script yielding the file name to unpack instead of the pat…

87e533c

…h into the bundle. Not sure why this code was written this way, should have never worked…

Properly setting permissions of archived app bundles

38a32a9

Cleaning up bundles adding multiple name entries

5d25b28

Adding Mac thin client manifests

927bf38

Fixing bundle flag

abbc6fe

Client path does not include trailing slash. Checking for a .app suffix instead.

Initial work on macBundleExternal/ Internal keys

4fc98e2

Elimating dir checks for bundle

ca85d8d

Apply suggestions from code review

28e387d

Co-authored-by: Adam Johnson <[email protected]>

Apply suggestions from code review

f956772

Co-authored-by: Adam Johnson <[email protected]>

Fixing typo in string literal

8e14b72

Eliminating manual folder extension check

c1ac1ed

colincornaby force-pushed the mac-patch branch from 472f0ec to c1ac1ed Compare July 18, 2024 06:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Initial work on Mac patching #12

Initial work on Mac patching #12

colincornaby commented Oct 15, 2023

colincornaby commented Oct 15, 2023

Hoikas Oct 15, 2023

colincornaby Oct 15, 2023

colincornaby commented Oct 22, 2023

Hoikas left a comment

colincornaby commented Oct 23, 2023

colincornaby Jun 2, 2024

colincornaby commented Jun 30, 2024

colincornaby commented Jul 1, 2024

Hoikas Jul 2, 2024

colincornaby Jul 2, 2024

Hoikas Jul 14, 2024

Hoikas Jul 2, 2024

colincornaby Jul 2, 2024

Hoikas Jul 2, 2024

colincornaby Jul 2, 2024

Hoikas Jul 2, 2024

Hoikas Jul 2, 2024

colincornaby Jul 2, 2024

dpogue Jul 2, 2024

Hoikas Jul 14, 2024

dpogue Jul 2, 2024

dpogue Jul 2, 2024

colincornaby Jul 2, 2024

colincornaby commented Jul 2, 2024

Hoikas commented Jul 2, 2024

colincornaby commented Jul 3, 2024

colincornaby commented Jul 7, 2024

Hoikas left a comment

Hoikas Jul 14, 2024

Hoikas Jul 14, 2024

		"macInternal": _manifests("MacThinInternal", None, "MacInternal"),
		"macExternal": _manifests("MacThinExternal", None, "MacExternal"),

		"macexternal": _directorytuple("", "client/mac/external"),
		"macinternal": _directorytuple("", "client/mac/internal"),

		"plasma-macos-x64-internal-release": "macInternal",
		"plasma-macos-x64-external-release": "macExternal",

Initial work on Mac patching #12

Are you sure you want to change the base?

Initial work on Mac patching #12

Conversation

colincornaby commented Oct 15, 2023

colincornaby commented Oct 15, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

colincornaby commented Oct 22, 2023

Hoikas left a comment

Choose a reason for hiding this comment

colincornaby commented Oct 23, 2023

Choose a reason for hiding this comment

colincornaby commented Jun 30, 2024

colincornaby commented Jul 1, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

colincornaby commented Jul 2, 2024

Hoikas commented Jul 2, 2024

colincornaby commented Jul 3, 2024

colincornaby commented Jul 7, 2024

Hoikas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment