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

ARROW-5165: [Python] update dev installation docs for --build-type + validate in setup.py #4192

Conversation

jorisvandenbossche
Copy link
Member

  • Remove the mention of --build-type=$ARROW_BUILD_TYPE in the docs, since ARROW_BUILD_TYPE is not set, and 'release' is already the default.
  • Validate the value of self.build_type so that doing --build-type=$ARROW_BUILD_TYPE without having the env variable set gives a helpful error message instead of failing when bundling includes

@jorisvandenbossche jorisvandenbossche changed the title ARROW-5178: [Python] update dev installation docs for --build-type + validate in setup.py ARROW-5165: [Python] update dev installation docs for --build-type + validate in setup.py Apr 23, 2019
python/setup.py Outdated
@@ -171,6 +171,11 @@ def initialize_options(self):
'gandiva']

def _run_cmake(self):
# check if build_type is correctly passed / set
if self.build_type not in ('release', 'debug'):
Copy link
Contributor

Choose a reason for hiding this comment

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

I would lowercase this as sometimes it is passed as "RELEASE" or "RelWithDebInfo". Also note there are more value than this, see https://cmake.org/cmake/help/v3.0/variable/CMAKE_BUILD_TYPE.html

Copy link
Member Author

Choose a reason for hiding this comment

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

OK on the lower case.

I can also remove the check in its entirety, if it would be too strict (don't know if those other build types are used within arrow development). I mainly wanted to address the issue described in https://issues.apache.org/jira/browse/ARROW-5165 that the error you now get is rather confusing.

Copy link
Member

@pitrou pitrou left a comment

Choose a reason for hiding this comment

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

+1 from me.

@pitrou pitrou closed this in a419ec8 Apr 24, 2019
@pitrou
Copy link
Member

pitrou commented Apr 24, 2019

@jorisvandenbossche If you want to make a deeper dive in the development docs and fix/refresh them, you're welcome :-)

@jorisvandenbossche jorisvandenbossche deleted the ARROW-5178-python-arrow-build-type branch April 24, 2019 09:14
@jorisvandenbossche
Copy link
Member Author

If you want to make a deeper dive in the development docs and fix/refresh them, you're welcome :-)

They are actually already quite nice, but will certainly report issues / make fixes while going through them.

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

Successfully merging this pull request may close these issues.

3 participants