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

Dynamically generate valid xcode project files from json #9

Closed
wants to merge 96 commits into from

Conversation

BPerlakiH
Copy link
Collaborator

@BPerlakiH BPerlakiH commented Dec 15, 2023

Fixes #1
Fixes kiwix/kiwix-apple#603
Fixes kiwix/kiwix-apple#559
Fixes #10
Fixes kiwix/kiwix-apple#615

How it works

  • checkout the main Kiwix apple branch, from kiwix/apple (currently it is using a dedicated branch, but once that's merged we can use main).
  • checkout this repository
  • run the built in scripts to generate: info plist files, config files, and finally generate a custom project file that is re-using all the settings from the kiwix/apple main branch, but adding custom targets:
Screenshot 2023-12-31 at 15 06 12

next steps

  • the next step is to build the apps from the generated project using fastlane.

other changes

  • it is renaming the DWDS folder to be lowercase: dwds
  • it is adding python scripts to do most of the parsing / transforming / downloading steps, with unit tests

@BPerlakiH BPerlakiH marked this pull request as draft December 18, 2023 03:29
@BPerlakiH
Copy link
Collaborator Author

Thank you!

I think we lack explanations regarding how this technically works: I might have missed something but it looks like any push to main or PR will trigger a build and testflight beta for all custom apps. Is that what we want?

There doesn't seem to be a way to (even manually) trigger a single app build as well. It even looks like it's done step by step for all apps so a broken app would fail the whole thing. Having a single-app tool would allow to run it in parallel in github actions and even have a per-app reporting in the workflow.

On the python code, I have some general comments

  • generate_and_download.py: I don't see the value of this file at the moment. If it's entirely dependent on custom_apps.py and is entirely static code, maybe it should be move in __main__ of custom_apps.py
  • you should use pathlib module for filepath handling and not rely on strings. This will be handy for stuff like rglob
  • There's no dosctring and naming is not helping: CustomApps().append_to(): no clue from naming what this is supposed to do.
  • the chaining of string replacements eventually ending up being executed is terrifying 😅 This is extremely opaque and hard to maintain.
  • use f-strings instead of .format() ; it's clearer and in line with our convention.
  • split the Info holder and the command builder.
  • dosctring or comments for all commands
  • fix codefactor issues
  • use named groups for your regexp
  • there's a TODO:remove me comment. is it still necessary?
  • don't us the shell to perform simple operations for which there's an API (file copy for instance)
  • use subprocess (with arrays) when running processes

I have fixed these issues, please review.
Next step is to look into the CI / CD separation, as mentioned above.

@kelson42
Copy link
Contributor

@BPerlakiH CI still fails!

Copy link
Member

@rgaudin rgaudin left a comment

Choose a reason for hiding this comment

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

Thank you ; it's better but I think the priority should go into making it per-app first and then improve the code quality.

def append_to(custom_plist="Custom.plist"):
for cmd in InfoParser.plist_commands():
os.system("{} {}".format(cmd, custom_plist))
def create_custom_project_file(self, path=Path()/'custom_project.yml'):
Copy link
Member

Choose a reason for hiding this comment

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

You should not set it here as this is set once at import time. Better to leave it None and in the function set it if it's None

# create the xcconfig files
custom_apps.create_xcconfigs()
# create the plist files
custom_apps.create_plists(custom_plist=Path()/"Custom.plist")
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
custom_apps.create_plists(custom_plist=Path()/"Custom.plist")
custom_apps.create_plists(custom_plist=Path("Custom.plist"))

out_path = self._info_plist_path()
# create dir, if doesn't exists yet
if not out_path.parent.exists():
out_path.parent.mkdir()
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
out_path.parent.mkdir()
out_path.parent.mkdir(parents=True, exists_ok=True)

then you dont need the condition


def create_plist(self, based_on_plist_file):
with based_on_plist_file.open(mode="rb") as file:
plist = plistlib.load(file)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
plist = plistlib.load(file)
plist = plistlib.load(based_on_plist_file.read_bytes())



def _info_plist_path(self):
return Path()/self.brand_name/f"{self.brand_name}.plist"
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
return Path()/self.brand_name/f"{self.brand_name}.plist"
return Path(self.brand_name) / f"{self.brand_name}.plist"

|
cd apple
bundle exec fastlane ios beta
# bundle exec fastlane mac beta
Copy link
Contributor

Choose a reason for hiding this comment

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

  • Not sure why all these secrets are needed to just build the app and not upload it!
  • Why is macOS not tested?


## At the beginning we have the following:
- custom brand folders, each with an info.json file, and xcassets
- we have the kiwix/apple code (as a seperate repository).
Copy link
Contributor

Choose a reason for hiding this comment

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

kiwix/apple1 or link.


## What we need in order to build a custom app?

### A Zim file
Copy link
Contributor

Choose a reason for hiding this comment

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

ZIM file

This needs to be downloaded from the url given in the `info.json`, and placed under the branded folder, eg: `/custom/dwds`

### A .plist file
The plist file is a list of settings, that is used by xcode for the given target to be built. It is in a required xml format.
Copy link
Contributor

Choose a reason for hiding this comment

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

Xcode

### A .plist file
The plist file is a list of settings, that is used by xcode for the given target to be built. It is in a required xml format.

It is created under the folder with the same name, eg.: `dwds/dwds.plist`,
Copy link
Contributor

Choose a reason for hiding this comment

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

This file is automatically generated and should not be edit manually. This should be clear.


# A new custom_project.yml file
The main `kiwix/apple` repo contains a `project.yml` file, it describes all the common build and project settings, and all the templates we re-use to create a final xcode project file, which is used to build the custom apps. At the end each custom app will be a separate target we can build.
Therefore we need dynamically create the `custom_project.yml` file, which will import the `project.yml`, and sets up the new targets (one for each brand), it looks more or less, like this:
Copy link
Contributor

Choose a reason for hiding this comment

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

we dynamically create

@@ -0,0 +1,59 @@
from pathlib import Path
Copy link
Contributor

Choose a reason for hiding this comment

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

The code should be for only one specific custom app. If we want to make all custom apps at the same time, this will be done at a higher level.

@@ -0,0 +1,13 @@
{
Copy link
Contributor

Choose a reason for hiding this comment

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

Please make a fake app (not like a real one with real values). We should also avoid to download such a big file. I have prepared here a small test ZIM file https://dev.kiwix.org/apple/custom/test/wikipedia_en_ray_charles_maxi_2023-12.zim

@@ -0,0 +1,80 @@
# Technical documentation for contributors
Copy link
Contributor

Choose a reason for hiding this comment

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

Usually this is called CONTRIBUTING.md I believe.

@BPerlakiH
Copy link
Collaborator Author

The required changes were merged in other PRs.

@BPerlakiH BPerlakiH closed this Jan 15, 2024
@BPerlakiH BPerlakiH deleted the feature/1-DWDS-build-script branch February 14, 2024 21:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants