AlexAplin / nndownload

Download and process links from Niconico (nicovideo.jp)
MIT License
213 stars 28 forks source link

Fix build properly #156

Closed fireattack closed 3 months ago

fireattack commented 3 months ago

To be blunt, the current project structure quickly becomes a mess if we constantly apply band-aid fixes.

In this PR I will try my best to revert some changes we had lately back to sane practice, while still keep pyinstaller for Win action work.

Go back to use ./{packagename} structure.

This is the still the most used, most standard structure.

./src approach exists, but the "proper" way to use ./src is actually something like ./src/{packagename}/..., which I don't think we would bother.

Stick with relative import.

We're eventually going to refactor the whole thing with class and submodules. It's better we stick with relative imports now to save us some headaches ahead.

That means you can't just run nndownload/nndownload.py (which was already broken for a while after moving files from root to src) but I added a proper __main__.py so you can always run python -m nndownload. And you can debug it as a module properly this way.

Fix package entry

Added __main__.py as said above. python -m nndownload behaves exactly like old python src/nndownload.py.

Also added a entry file at root level so you can just python nndownload.py which would just import from our package. This is also useful for build with Pyinstaller, see below.

Fix Pyinstaller action

Instead of adding absolute imports like from src.ffmpeg_dl import FfmpegDL, FfmpegDLException (#155), which would break in any other scenario other than in that specific action, I use the entry file nndownload.py I just added.

This way, we can use this entry file to build our executable without any import issues. This is also very future-proof since we can modify our package however we want without fiddling with our spec or action config.

Fix build

Move all the metadata file back to root. That includes requirements. txt and .spec for Pyinstaller builder.

Reverted setup.py back to old version, but also changed scripts to entry_points.

This is the best practice. Compared to a a hard-coded path to nndownload.py, it works without any issue with relative imports, and it would work on any OS (for example, on Windows, when you install via pip, it would automatically generate a wrapper Scripts\nndownload.exe (which is also by default in $path). This way, the end user can just run nndownload in any path, like any CLI applications, if you install from source.

And pip install --editable . finally works again.

AlexAplin commented 3 months ago

I appreciate your patience and efforts. It was never my intention to trudge up the structure this much before getting to the class rewrite, but people wanted their Windows builds. 🥲

fireattack commented 3 months ago

I appreciate your patience and efforts. It was never my intention to trudge up the structure this much before getting to the class rewrite, but people wanted their Windows builds. 🥲

Understandable, that's also why I made sure it still works: https://github.com/fireattack/nndownload/actions/runs/8310821649 [I tested this on Windows].

There is actually another windows-only bug I discovered and fixed, but that's not because of the build.

I will push another fix for that once this one is merged due to structure change.