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

Refactor setup.py to build and stage into temp area for setup.py to install from #956

Merged
merged 15 commits into from
Apr 29, 2021

Conversation

reinecke
Copy link
Collaborator

@reinecke reinecke commented Apr 28, 2021

Refactors setup.py to properly leverage the newly cleaned up cmake builds (reworked in #837).

(Note: this replaces #955 that wouldn't reopen due to GitHub oddness)

Summarize your change.

Our setup.py had been arranged to try and figure out the final install location for for the libraries and place products in there. The python packaging utilities actually want you to assemble your build into a temp location where it will then pick it up to either package or install to the proper place.

This refactor now tells the cmake build to install into that temp location. Because of this, we have to manually maintain much less code that supports determining the user's desired end action with the assembled package (e.x. install, install using --user, bdist, bdist_wheel).

Other changes include:

  • Removed _Ctx class instance that was storing build config as a global - it made it hard to follow the flow of code and would likely be prone to bugs.
  • Removed setuptools and pip version checks per this conversation
  • Updated -jx flag generation to be based on detected number of system CPUs (better compile speeds!)

@reinecke reinecke marked this pull request as draft April 28, 2021 17:33
@ssteinbach ssteinbach changed the title Refactor setup.py for cmake updates Refactor setup.py to build and stage into temp area for setup.py to install from Apr 28, 2021
@codecov-commenter
Copy link

codecov-commenter commented Apr 28, 2021

Codecov Report

Merging #956 (a576a8a) into master (db99967) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #956   +/-   ##
=======================================
  Coverage   85.63%   85.63%           
=======================================
  Files         191      191           
  Lines       18160    18160           
  Branches     2061     2061           
=======================================
  Hits        15552    15552           
  Misses       2085     2085           
  Partials      523      523           
Flag Coverage Δ
unittests 85.63% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.


Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update db99967...a576a8a. Read the comment docs.

setup.py Show resolved Hide resolved
@reinecke reinecke marked this pull request as ready for review April 28, 2021 18:18
setup.py Show resolved Hide resolved
Copy link
Collaborator

@ssteinbach ssteinbach left a comment

Choose a reason for hiding this comment

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

One note about vs 2017 vs 2019 and a formatting note suggestion to match the rest of the file. Thanks!

setup.py Outdated Show resolved Hide resolved
setup.py Show resolved Hide resolved
setup.py Outdated Show resolved Hide resolved
@JeanChristopheMorinPerso
Copy link
Member

When some hacks are removed from a setup.py file, it makes me extremely happy 😁

Great work!

Copy link
Collaborator

@ssteinbach ssteinbach left a comment

Choose a reason for hiding this comment

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

This looks great. Anything else you want to change on this? Or is it ready to land?

@reinecke
Copy link
Collaborator Author

Let's land this one and move on! I'll do it now.

@reinecke reinecke merged commit 1f92548 into AcademySoftwareFoundation:master Apr 29, 2021
@ssteinbach ssteinbach added this to the Public Beta 14 milestone Jun 7, 2021
@meshula meshula added the build issues building OTIO label Jun 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build issues building OTIO
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants