-
Notifications
You must be signed in to change notification settings - Fork 26
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
Replaced compile scripts with mcprep-build #422
Conversation
(Saleman pitch time) mcprep-build is our new, state-of-the-art build system custom made for the MCprep project that's cross platform! Now we get less headaches from dealing with Bash and the Windows shell as this new build system is written in Python! Jokes aside, this new build system is much more maintainable then the old build system. I still need to add autopopulation, but it's 99% the same, minus fast mode. In the future I want to turn this into a general purpose bpy-addon-builder sorta thing, but let's not get ahead of ourselves.
MCprep build had some issues with no arguments, so I went ahead and fixed it since it would be annoying if we could only make dev builds of MCprep
This is to make the documentation consistant with the actual build system.
4d9d18a
to
bc8c363
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fantastic to already see this taking shape, nice work and thanks for picking this up. A few suggestions - bigger thing is the code issue on shutil make zip (sorry didn't have time to debug further, though also not entirely sure what you are intending for the root/ending dir structure, so weill be good to spot check that).
mcprep-build.py
Outdated
BUILD_NAME: str = "MCprep_addon" | ||
|
||
# Internals | ||
BUILD_DIRECTORY: Path = Path("build") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just checking - if you run MCprep/mcprep-build.py
this would result in build
being a level up than if you cd'd into MCprep and ran mcprep-build.py
right?
If possible, would be great if we can force these paths to start relative to where this current script is saved from, vs the cwd. We could set another global up higher which is SCRIPT_DIR: str = os.path.dirname(__file__)
(or the equivalent using the Path module ofc)
mcprep-build.py
Outdated
debug_mode: bool = False | ||
|
||
# Check arguments to see what mode to run the compiler in | ||
if len(sys.argv) <= 1: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you familiar with argparse
? I wish I learned about it a long time ago when setting up the scripts originally, makes things like this so much better. You even get auto documentation on commandline and erroring for unsupported args. And it's clearer to read generally.
https://docs.python.org/3/library/argparse.html
So we'd have:
- env: positional arg for prod or dev (which we will eventually pair with avoiding have dev be a hard coded symbol in the codebase, a pain during releasing)
- debug: optional arg
- autoinstall: optional arg, maybe even default off to be safe for new developers who might have custom rigs installed to their MCprep addon folder?
- fast: when we get there, for fast build. Maybe a better name could be given.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks far better then manually handling arguments, I'll implement that ASAP
mcprep-build.py
Outdated
|
||
# Check if the developer blender_installs.txt | ||
if not BLENDER_INSTALLS.exists(): | ||
print("You need a blender_installs.txt file!") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be amazing if we could add in a y/n question to auto generate it here (similar to how I auto generate the file here).
Even if we don't quite get to auto populating the install locations yet in this commit (which I'm supportive of, as that will bloat the PR), we could at least just create the file in the right place so it's one less step for someone getting started.
And if we're already creating the files, then why not create the blender_execs.txt
as well, even if it's not used by this script. This all could be even moved into an "onboarding" function which gives more instruction for new developers on where and how to get started for building the first time and directing them to try and run tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Auto population should be easy to implement with some dictionaries, I just need to figure out how to get all the paths that match a wildcard
mcprep-build.py
Outdated
for line in f: | ||
blender_installs.append(Path(line.rstrip())) | ||
if not len(blender_installs): | ||
print("No autopopulation of Blender install paths for now!") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just FYI this feels a bit backwards to me to expect autopopulation here anyways. My take is that if the text file exists and can be read, it should be treated as truth (but raise an error if empty). That way someone either uses auto population when first setting up, or they're doing it manually. So I'd just write a print instead something like "No install paths specified in blender_installs.txt
"
mcprep-build.py
Outdated
|
||
# Create archive and move it to the build directory since shutil makes | ||
# the archive in the current working directory | ||
shutil.make_archive(BUILD_NAME, "zip", ADDON_DIRECTORY) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't this copy all of the git hidden folders and whatnot?
We should probably include an ignore or include list here. More to maintain, but helps enforce consistent building by different individuals (and maybe it's eventually automated on a server while doing releases, all the better to ensure no locality differences).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Depends on the addon folder. MCprep's code is in a seperate folder, so only the actual code is copied
mcprep-build.py
Outdated
|
||
# Install addon | ||
for path in blender_installs: | ||
if not path.exists(): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there should be a step here which removes the existing file location, so that leftover files aren't hanging around. ie equivalent of this.
mcprep-build.py
Outdated
BUILD_DIRECTORY: Path = Path("build") | ||
INTERMIDIATE_PATH: Path = BUILD_DIRECTORY / Path("inter") | ||
ADDON_DIRECTORY: Path = Path("MCprep_addon") | ||
BLENDER_INSTALLS: Path = Path("blender_installs.txt") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
May as well also define BLENDER_EXECS, to help reinforce the concept.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I dunno, this is mostly a building script so I don't think it makes sense to add BLENDER_EXECS
mcprep-build.py
Outdated
@@ -0,0 +1,101 @@ | |||
import os |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be good to docstring the args at the top (even if this is mitigated by argparse once utilized)
mcprep-build.py
Outdated
f.write("This is the MCprep Dev File created by mcprep-build!") | ||
|
||
# Rebuild | ||
shutil.make_archive(BUILD_NAME, "zip", INTERMIDIATE_PATH / ADDON_DIRECTORY) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI I just ran locally and it got hitched here:
mcprep $ python3 mcprep-build.py -d
Traceback (most recent call last):
File "/Users/patrickcrawford/Documents/gits/mcprep/mcprep-build.py", line 101, in <module>
main()
File "/Users/patrickcrawford/Documents/gits/mcprep/mcprep-build.py", line 88, in main
shutil.make_archive(BUILD_NAME, "zip", INTERMIDIATE_PATH / ADDON_DIRECTORY)
File "/opt/homebrew/Caskroom/miniforge/base/lib/python3.9/shutil.py", line 1072, in make_archive
os.chdir(root_dir)
FileNotFoundError: [Errno 2] No such file or directory: 'build/inter/MCprep_addon'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I think I just need to remove ADDON_DIRECTORY
Co-authored-by: Patrick W. Crawford <theduckcow@live.com>
Note to self: add Flake8 checking as well |
As suggested, I've added a SCRIPT_PATH variable to represent the path of the mcprep-build script, just to make sure that all future paths are correct. In addition, I've also removed the ADDON_DIRECTORY variable on line 88 since it's incorrect logic (we're trying to zip a folder that shouldn't exist)
1c665c5
to
419d706
Compare
We'll be using flake8 in MCprep build to perform PEP8 code checks to make sure our code complies with PEP8 guidelines.
Ok added flake8 to my local build, but...: /home/USER/Documents/MCprep/MCprep_addon/spawner/spawn_util.py:915:1: W191 indentation contains tabs
/home/USER/Documents/MCprep/MCprep_addon/spawner/spawn_util.py:916:1: W191 indentation contains tabs
/home/USER/Documents/MCprep/MCprep_addon/spawner/spawn_util.py:925:1: W191 indentation contains tabs
/home/USER/Documents/MCprep/MCprep_addon/spawner/spawn_util.py:926:1: W191 indentation contains tabs
/home/USER/Documents/MCprep/MCprep_addon/spawner/spawn_util.py:927:1: W191 indentation contains tabs
/home/USER/Documents/MCprep/MCprep_addon/spawner/spawn_util.py:928:1: W191 indentation contains tabs
/home/USER/Documents/MCprep/MCprep_addon/spawner/spawn_util.py:929:1: W191 indentation contains tabs
/home/USER/Documents/MCprep/MCprep_addon/spawner/spawn_util.py:930:1: W191 indentation contains tabs
/home/USER/Documents/MCprep/MCprep_addon/spawner/spawn_util.py:931:1: W191 indentation contains tabs
/home/USER/Documents/MCprep/MCprep_addon/spawner/spawn_util.py:932:1: W191 indentation contains tabs
/home/USER/Documents/MCprep/MCprep_addon/spawner/spawn_util.py:933:1: W191 indentation contains tabs
/home/USER/Documents/MCprep/MCprep_addon/spawner/spawn_util.py:934:1: W191 indentation contains tabs
/home/USER/Documents/MCprep/MCprep_addon/spawner/spawn_util.py:935:1: W191 indentation contains tabs
/home/USER/Documents/MCprep/MCprep_addon/spawner/spawn_util.py:936:1: W191 indentation contains tabs
/home/USER/Documents/MCprep/MCprep_addon/spawner/spawn_util.py:937:1: W191 indentation contains tabs
/home/USER/Documents/MCprep/MCprep_addon/spawner/spawn_util.py:938:1: W191 indentation contains tabs
/home/USER/Documents/MCprep/MCprep_addon/spawner/spawn_util.py:939:1: W191 indentation contains tabs
/home/USER/Documents/MCprep/MCprep_addon/spawner/spawn_util.py:940:1: W191 indentation contains tabs
/home/USER/Documents/MCprep/MCprep_addon/spawner/spawn_util.py:941:1: W191 indentation contains tabs
/home/USER/Documents/MCprep/MCprep_addon/spawner/spawn_util.py:946:1: W191 indentation contains tabs
/home/USER/Documents/MCprep/MCprep_addon/spawner/spawn_util.py:947:1: W191 indentation contains tabs
/home/USER/Documents/MCprep/MCprep_addon/spawner/spawn_util.py:951:1: W191 indentation contains tabs
/home/USER/Documents/MCprep/MCprep_addon/spawner/spawn_util.py:952:1: W191 indentation contains tabs Should we just ignore warning |
Ok so ignoring /home/USER/Documents/MCprep/MCprep_addon/__init__.py:37:80: E501 line too long (80 > 79 characters)
/home/USER/Documents/MCprep/MCprep_addon/__init__.py:38:80: E501 line too long (90 > 79 characters)
/home/USER/Documents/MCprep/MCprep_addon/__init__.py:54:1: E402 module level import not at top of file
/home/USER/Documents/MCprep/MCprep_addon/__init__.py:57:19: F821 undefined name 'load_modules'
/home/USER/Documents/MCprep/MCprep_addon/__init__.py:61:1: F401 'bpy' imported but unused
/home/USER/Documents/MCprep/MCprep_addon/__init__.py:61:1: E402 module level import not at top of file
/home/USER/Documents/MCprep/MCprep_addon/__init__.py:63:1: E302 expected 2 blank lines, found 1
/home/USER/Documents/MCprep/MCprep_addon/addon_updater.py:176:3: E722 do not use bare 'except'
/home/USER/Documents/MCprep/MCprep_addon/addon_updater.py:206:5: E128 continuation line under-indented for visual indent
And much more... God I love PEP8 EDIT: remove most of the error so the browser doesn't become slow |
This is so we get PEP8 errors at compile time God I love PEP8 but we got an ungodly amount of errors, and that's ignoring W191 (Tabs instead of Spaces)
Oh yes, we definitely have a lot of pep8 warnings, as this style has only more recently been used in practice. Hence the focus on incremental improvements with blocks of code that are modified. Changing everything at once would again create the issue of making it very hard to QA. Yes the whitespace once I do have ignored in my local config. If we want to revert to spaces instead of tabs at somepoint, it should be at a later time when no other lines of code are in progress (as again, for the same reason, changes every single line of code, making merging or in progress PRs impossible to merge without some serious headache) |
One more quick thing @StandingPadAnimations - I checked out the branch to see how it's going, we're getting closer, but when I add this diff.. @@ -97,6 +97,7 @@ def main():
f"Path {str(path)} in blender_installs.txt doesn't exist, skipping..."
)
shutil.unpack_archive(BUILT_ZIP, path)
+ print(f"Installed to {path}")
if __name__ == "__main__": ... I get results like:
Looks like all the files are being installed into the addons folder, instead of to a nested MCprep folder. Is that happening to you? I wouldn't expect anything here should differ by operating system. Please re-request when it's working and you'd like me to do the followup review! |
Originally the build script unzipped to the paths in blender_installs.txt directly, but that was incorrect logic and ended with the addons folder being polluted. This was an easy fix and I'm not sure how it slipped from my mind. In addition, I ran flake8 and fixed some formatting issues in the builder.
Alright I've recently been working on a more "general" builder called https://github.com/StandingPadAnimations/bpy-build. It's based off of MCprep build and has most of the same features (except debug files, I have an interesting alternative). Unlike 2023-05-14.13-21-23.mp4No auto-population, no extra files for users to deal with. If the version isn't installed, it's just skipped. Once I get the |
I will eventually be pushing it to Pypi so once finished, it should be as simple as adding it to |
Testing it on MCprep (the test MCprep_addon folder I used is empty) it does indeed work (although I need to remove some debug statements) |
bpy-build is a better version of mcprep-build that I'm working on. It's more general purpose but has far better configuration. bpy-build does work on the MCprep repo, I just need to finish some shortcommings
bpy-build now supports adding files during a build, which means we can now use bpy-build instead of mcprep-build (if we choose to do so).
Here's an example of 2023-05-14.15-44-14.mp4 |
I think I'll now be moving this completely to bpy-addon-build (slight rename due to bpy-build being taken on PyPi). It does mean an extra dependency that has to be installed by developers, but bpy-addon-build is much easier to use in comparison, and has a much better cli interface. |
bpy-addon-build is the better version of mcprep-build (the former being based off the latter) and will be maintained better, so it makes no sense having a seperate build system. bpy-addon-build has the following advantages: - YAML config - Auto-installation without blender_installs.txt - Ability to define actions and run them in specific cases - Better terminal output - Better argument handling
Flake8 is important for formatting checks so I've added it as a default action to be executed during the build process
bpy-addon-build was updated to better handle missing YAML config options, so this update is necesary
This version removes a debug statement... Maybe I should make a proper test suite
@StandingPadAnimations can you sync this with the milestone branch so that we resolve any conflicts, just to make sure everything's working well? I'll try right after then. |
PrepOptions is a dataclass that has a static set of options; no new options can be added at runtime. Since that's a gurantee about the class, we can use `__slots__` to make sure that typos in field names generate an error. In addition, `__slots__` has a performance and memory benefit, but that's minor compared to the development improvement that would be gained. Due to Blender 2.8 using Python 3.7, we have to manually add the slots ourselves in the class definition. Hopefully in a future MCprep release we can remove this manual declaration and use the `slots` option in the dataclass decorator.
(Saleman pitch time) mcprep-build is our new, state-of-the-art build system custom made for the MCprep project that's cross platform! Now we get less headaches from dealing with Bash and the Windows shell as this new build system is written in Python! Jokes aside, this new build system is much more maintainable then the old build system. I still need to add autopopulation, but it's 99% the same, minus fast mode. In the future I want to turn this into a general purpose bpy-addon-builder sorta thing, but let's not get ahead of ourselves.
Updated now |
Hm odd it still says there's a merge conflict, did you sync in |
Yeah, it's due to |
Oh durr, that makes sense. Thanks 👍 Will let you know if I run into anything |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comments below, but also see this issue .
..and per comment below, I'm a bit confused about the state of the PR. Should the mcprep-build.py file actually call bpy-addon-build instead of making the zips itself..?
CONTRIBUTING.md
Outdated
@@ -45,11 +44,9 @@ As above, a critical component of maintaining support and ensuring the wide numb | |||
|
|||
### Compile MCprep using scripts | |||
|
|||
Scripts have been created for Mac OSX (`compile.sh`) and Windows (`compile.bat`) which make it fast to copy the entire addon structure the addon folders for multiple versions of blender. You need to use these scripts, or at the very least validate that they work, as running the automated tests depend on them. | |||
MCprep uses the [bpy-addon-build](https://github.com/StandingPadAnimations/bpy-build) package to build the addon, which makes it fast to copy the entire addon structure the addon folders for multiple versions of blender. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we're going to enforce users to install another build system, we should make it a straight forward as possible. Let's make it a step by step here of the commands to write (even if, yes, it's mostly copy-paste from the source repo):
Something like the following I gather:
- Must have a local install of python (what versions are supported? Ranges?)
- Create a virtual environment using virtual env:
- Install virtual env if necessary:
python3 -m pip install --user virtualenv
(hmm.. this too is an extra barrier to entry, now suddenly people have to know how to use virtual environment, even if it is best practice) - Create your environment:
python37 -m venv ./venv # Creates env called venv
- Install virtual env if necessary:
- Enable your virtual environment.
- On Windows:
.\venv\Scripts\activate
- On OSX/linux:
source venv/bin/activate
(orsource .venv/bin/activate
)
- On Windows:
- Run
pip install bpy-addon-build
- Moving forward, you can now build the addon by calling
bpy-addon-build -b dev
(to install the dev version)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, is mcprep_venv_*
meant to be your virtual environment name? I suppose we should use that above instead of venv
since we already have this in the gitignore (would be nicer if we could update the gitignore to be mcprep_venv*
so it could also match mcprep_venv
itself)
MCprep_addon/materials/generate.py
Outdated
__slots__ = ("passes", | ||
"use_reflections", | ||
"use_principled", | ||
"only_solid", | ||
"pack_format", | ||
"use_emission_nodes", | ||
"use_emission") | ||
passes: dict[str, str] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This typed dict creates syntax errors in blender 2.80, and regardless I don't see how is related to the PR topic itself. At the very least, will need to change this to support typing that was around in python3
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was added in the milestone branch and was synced up
bpy-build.yaml
Outdated
|
||
during_build: | ||
default: | ||
- [flake8 --extend-ignore W191 .] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see now this is included, which is good - but for some reason, I'm still getting W191 printouts (completely taking up my screen).
default: | ||
- [flake8 --extend-ignore W191 .] | ||
dev: | ||
- create_file("mcprep_dev.txt") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does this do? Just creates the empty file, for the addon to recognize run as a dev build?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, it just creates an empty file
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might be good to add a yaml comment inline which clarifies why we do this. Maybe:
# Create dev file to force dev tracking and other behavior
poetry.lock
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need documentation on how to make use of poetry files, right now not sure if I'm even utilizing it. Should also document how this gets updated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The poetry docs are a good place to start: https://python-poetry.org/docs/basic-usage/
Co-authored-by: Patrick W. Crawford <theduckcow@live.com>
Having issues where it's not providing the intended result, still showing W191. Plus, Blender operator properties are not defined using a syntax that lint checkers can parse, creating a lot of noise. We'll need to rethink how to adopt this in the future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're close, but one thing:
- The one blocking change: Fixing of Need extra level of folder nesting in zip build bpy-build#4
- Right now, the generate zip is directly the files instead of a folder containing the files. It; borqs up the blender addon folder instead of creating a subdir for the plugin. Don't want to merge this until we know it makes valid zip builds.
- Technically this is a change not in this branch, but the build script; so consider this actual branch approved until that change is made.
Nice to haves, but can do later.
- Making the flake ignore W191 actually work, I'm still getting printouts for every line. Not sure why. If I manually run
flake8 --extend-ignore W191
I get more what I expect. I'm also finding flake unfortunately not working great, since it treats the way blender requires us to handle operators as invalid syntax (and then errors on all following lines).- I've just pushed a commit which disables flake8 for now, let's add it back later as a step two once we can figure a way around this.
- Adding a "-fast" build option would be very nice to add, whereby we skip copying over resources.
- How do we think we can get this back? Otherwise, running repeated tests will be a huge burden (a single test would take only a second or two to run, but if doing a full build before each time, since the test script calls compiling, it makes it very annoying).
I realize we also want to add in the build step which runs the |
Ok so a part of me wants to migrate from YAML to TOML for the build config. I've been working on https://github.com/StandingPadAnimations/kubo in my spare time, and it uses TOML. TOML would make build files feel less weird, and would give more flexibility (right now build actions consist of horrific regex and if statements 🥲). We can do a TOML migration after 3.5 though. A part of me also wants to rewrite |
Let's keep it simple and just try to address the minimum here, since it's blocking moving ahead - think you'll be able to address the zip folder structure issue next couple days? EDIT: I also somewhat feel it's an extra barrier to entry having another build repo, I'd like to not throw another language in the mix. |
Should be fixed now, hopefully (this honestly was a pain to figure out, and the workaround isn't great, but oh well)
I think it would be best to discuss this on the EDIT: you'll need to update |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are welcome to merge at your convenience! (ideally once pypi is updated) Will be great to have this merged in and working full, nice work creating a convenient to use build system
As mentioned on Moo-Ack-Productions/bpy-build#5 (comment), |
(Saleman pitch time)
mcprep-build is our new, state-of-the-art build system custom made for the MCprep project that's cross platform! Now we get less headaches from dealing with Bash and the Windows shell as this new build system is written in Python!
Jokes aside, this new build system is much more maintainable then the old build system. I still need to add autopopulation, but it's 99% the same, minus fast mode.