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

Create modern python package for HaikuPorter #259

Open
wants to merge 24 commits into
base: master
Choose a base branch
from

Conversation

jurgenwigg
Copy link

@jurgenwigg jurgenwigg commented Jul 11, 2023

Main goal is to create proper python package which could be installed via pip install command. At this moment package is created in a such way that it is not possible to install package flawless on newer python versions.

Creating unit tests, doing cleanup and removing all python2.7 leftovers, enabling lint, types and other static code analysis will be done under separate PR.

Copy link
Member

@scottmc scottmc left a comment

Choose a reason for hiding this comment

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

Why the rename? I really hate having_under_scores_in_files_names
It's been called haikuporter since the day it was created.

@jurgenwigg
Copy link
Author

@scottmc I had to rename file due to issue I had on my computer. MacOS is case insensitive and it caused that while cloning the repository I had a big problem. File names will be as in the original repository - haikuporter. But file itself will be .py file instead of HaikuPorter or I can create directory for the source code of haikuporter itself. Both options are good but it depends on our decision.

In terms of future changes, for Python PEP8 suggests pretty clearly how to style names for classes, functions, etc. Enabling checking formatting for the python code would require setting custom RC file for pylint or changing the names to comply with PEP8 standard. This is open point for future PR.

@pulkomandy
Copy link
Member

with some reorganization of the sources it should be possible to keep the name as "haikuporter", lowercase, for both the file and the directory. The file is meant to be installed in some /bin directory while the directory is meant to be a python module.

I think there will be a lot of people running haikuporter directly from the git repository, but they can probably handle some reorganization.

I agree that renaming everything to pep8 is a good idea, but we can still keep the "haikuporter" name :)

@jurgenwigg
Copy link
Author

We're all on the same page :) my idea was to preserve whole functionality as it was before but in modern python package with all advantages of newer version and with possibility to release it on pypi :)

@jurgenwigg
Copy link
Author

After migration HaikuPorter seems to work as before.

Importing module in the python itself:

» python
Python 3.11.4 (main, Jun  7 2023, 12:45:49) [GCC 9.4.0] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> import HaikuPorter
>>> import haikuporter
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
ModuleNotFoundError: No module named 'haikuporter'
>>> import haiku_porter
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
ModuleNotFoundError: No module named 'haiku_porter'
>>>

Executing haikuporter script from CLI:

» ./haikuporter mesa -j4
Error: Unable to find haikuports.conf in known search paths.
Error: See haikuports-sample.conf for more information

Migration to src based structure instead of flat one requires installation of HaikuPorter package with pip install . or pip install -e ..

@jurgenwigg jurgenwigg changed the title Draft: Create modern python package for haiku porter Create modern python package for HaikuPorter Jul 14, 2023
@waddlesplash
Copy link
Member

Haiku itself must be built on a case-sensitive system. You can make a case-sensitive disk in MacOS to check the repository out on and work on it that way.

@@ -0,0 +1,10 @@
repos:
Copy link
Member

Choose a reason for hiding this comment

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

What's all this about?

Copy link
Author

Choose a reason for hiding this comment

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

It came by default with cookie cutter template which I used to create/update this python package. It depends on if we would like to have git pre-commit hooks with formatting or not. Also basic checks are included in this step. In theory it should help to keep good-looking python code in terms of style before committing it to the repo.

CONTRIBUTING.rst Outdated
@@ -0,0 +1,151 @@
============
Copy link
Member

Choose a reason for hiding this comment

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

I think changes to README files should be done separately from such a refactor like this.

Copy link
Author

Choose a reason for hiding this comment

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

Good point - this file should be added in separate PR. I think it would be really good to have such guide how to contribute to this project.

Dockerfile Outdated
@@ -0,0 +1,21 @@
# syntax=docker/dockerfile:1
Copy link
Member

Choose a reason for hiding this comment

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

This file doesn't seem to have much purpose, perhaps it can be removed? BuildMaster has its own docker setup.

Copy link
Author

Choose a reason for hiding this comment

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

At this moment I agree that this file can be removed. Maybe in the future it would be helpful for developing HaikuPorter, i.e. some integration tests.

@jurgenwigg
Copy link
Author

The question is if HaikuPorter should be created in such way that it require case sensitive system? Proposed solution should work as previous package also in terms of case sensitive/insensitive. I don't see any obstacles in having haikuporter script 'as is' in root directory and HaikuPorter directory in src place.

@jurgenwigg
Copy link
Author

Ok, I've decided that this PR should cover only updating existing file structure to be able to install and create proper modern python package. Everything should work like before the update. Everything related to updating documentation (use mkdocs or sphinx), CI and pre-commit hooks should be done under separate PRs.

@waddlesplash
Copy link
Member

The question is if HaikuPorter should be created in such way that it require case sensitive system?

Using HaikuPorter for cross-compilation requires a Haiku build tree, which needs a case-sensitive filesystem. Using HaikuPorter on Haiku will always have a case-sensitive filesystem. So there's no reason to support case-insensitive ones.

@pulkomandy
Copy link
Member

But it's just moving a few files to an src/ subdirectory, so there is not really any problem with doing it anyways. And being able to checkout the git repository on a case insensitive filesystem sounds useful for many reasons (archiving, for example).

@waddlesplash
Copy link
Member

It's not just moving "a few files" but the entire sources, and furthermore due to the changes the renames aren't even tracked so we are losing all the history here.

I would prefer the haikuporter command/script be moved elsewhere (1 file) than all the others.

@jurgenwigg
Copy link
Author

It's not just moving "a few files" but the entire sources, and furthermore due to the changes the renames aren't even tracked so we are losing all the history here.

I would prefer the haikuporter command/script be moved elsewhere (1 file) than all the others.

The easiest way is just to add .py extension to existing haikuporter script. This is the easiest way to keep original directory for source files (HaikuPorter) and keep script file for it in the root directory. If this proposition would be a good deal for now? :)

@waddlesplash
Copy link
Member

Sounds fine.

@jurgenwigg
Copy link
Author

Now everything should work correctly and package itself should allow to install it through pip install . or pip install -e . command.

@Begasus
Copy link

Begasus commented Jul 17, 2023

Did a few check builds with these changes (only renamed the symlink in ~/config/non-packaged/bin to haikuporter instead of haikuporter.py (didn't want to change all the other calls to it or restart Terminal to be able to use it).
Things works fine for the buildprocess (not mingling with the internals). :)

@jurgenwigg
Copy link
Author

@waddlesplash @scottmc please check if your comments and findings were solved and if this PR can be merged :)

.gitignore Outdated

# From https://raw.githubusercontent.com/github/gitignore/main/Python.gitignore

Byte-compiled / optimized / DLL files
Copy link
Member

Choose a reason for hiding this comment

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

I think this is missing a #

Also probably some parts of this file are not needed.

Copy link
Author

Choose a reason for hiding this comment

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

This the base for further improvements. I've corrected missing #

@@ -0,0 +1,56 @@
.PHONY: install
install: ## Install the poetry environment and install the pre-commit hooks
@echo "🚀 Creating virtual environment using pyenv and poetry"
Copy link
Member

Choose a reason for hiding this comment

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

I think emojis are not well handled by Haiku terminal currently, maybe they should be avoided

Copy link
Author

Choose a reason for hiding this comment

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

The question is if we need this file anyway - maybe it can be removed? It came with the template.

Copy link
Member

Choose a reason for hiding this comment

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

Seems like not, it can be removed?

README.md Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
poetry.lock Outdated Show resolved Hide resolved

.history*
.vscode*
poetry.lock
*.pyc
*.pyo
recipeCache/
Copy link
Member

Choose a reason for hiding this comment

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

probably would be good to keep HaikuPorter-specific items at the top

@@ -1,29 +0,0 @@
"""Setup script for HaikuPorter
Copy link
Member

Choose a reason for hiding this comment

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

I think we actually use this to package HaikuPorter? Not sure...

Copy link
Author

Choose a reason for hiding this comment

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

setup.py is not needed and can be safely replaced by newer solution which is using pyproject.toml to install the package. Installing package with pip works flawlessly. I'll link article/document describing why setup.py is deprecated and how should it be replaced.

@@ -0,0 +1,102 @@
[tool.poetry]
Copy link
Member

Choose a reason for hiding this comment

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

Is this (and poetry.toml) really needed?

Copy link
Author

Choose a reason for hiding this comment

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

Poetry is like cargo in rust. You don't have to use it, but I treat it like a "touch of the future" where python has better package manager than pip ;)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants