Skip to content

ARROW-1464: [GLib] Add "Common build problems" section into the README.md of c_glib #1104

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.

Sign up for GitHub

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

Jump to bottom
Closed
wagavulin wants to merge 2 commits into from
Closed

ARROW-1464: [GLib] Add "Common build problems" section into the README.md of c_glib #1104

wagavulin wants to merge 2 commits into from

Conversation

@wagavulin
Copy link
Contributor

@wagavulin wagavulin commented Sep 16, 2017

Add some detailed explanation of common build problems especially on macOS because it requires some tweaks.

kou
kou reviewed Sep 17, 2017
View reviewed changes
c_glib/README.md Outdated
Copy link
Member

@kou kou Sep 17, 2017

Choose a reason for hiding this comment

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

You can use console for the case.

Copy link
Contributor Author

@wagavulin wagavulin Sep 18, 2017

Choose a reason for hiding this comment

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

OK.

kou
kou requested changes Sep 17, 2017
View reviewed changes
Copy link
Member

@kou kou left a comment

Choose a reason for hiding this comment

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

Thanks!
I added some comments!

c_glib/README.md Outdated
Copy link
Member

@kou kou Sep 17, 2017

Choose a reason for hiding this comment

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

following -> above?

Copy link
Contributor Author

@wagavulin wagavulin Sep 18, 2017

Choose a reason for hiding this comment

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

No. Warning: autoconf-archive 2017.03.21 is already installed, it's just not linked. ... is a warning message of brew install autconf-archive. And Linking /usr/local/Cellar/autoconf-archive/2017.03.21... is an error message of brew link autoconf-archive. So "following" is correct. But I think this paragraph may confusing, so I will rewrite.

c_glib/README.md Outdated
Copy link
Member

@kou kou Sep 17, 2017

Choose a reason for hiding this comment

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

ditto.

Copy link
Contributor Author

@wagavulin wagavulin Sep 18, 2017

Choose a reason for hiding this comment

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

OK.

c_glib/README.md Outdated
Copy link
Member

@kou kou Sep 17, 2017

Choose a reason for hiding this comment

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

Please add an empty new line after a section.

Copy link
Contributor Author

@wagavulin wagavulin Sep 18, 2017

Choose a reason for hiding this comment

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

OK.

c_glib/README.md Outdated
Copy link
Member

@kou kou Sep 17, 2017

Choose a reason for hiding this comment

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

Please use % for prompt. Because we already use % in this file.

Copy link
Contributor Author

@wagavulin wagavulin Sep 18, 2017

Choose a reason for hiding this comment

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

OK

c_glib/README.md Outdated
Copy link
Member

@kou kou Sep 17, 2017

Choose a reason for hiding this comment

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

Please add an empty line here.

Copy link
Contributor Author

@wagavulin wagavulin Sep 18, 2017

Choose a reason for hiding this comment

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

OK.

c_glib/README.md Outdated
Copy link
Member

@kou kou Sep 17, 2017

Choose a reason for hiding this comment

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

./configure PKG_CONFIG_PATH=$(brew --prefix libffi)/lib/pkgconfig

is better because configure keeps the PKG_CONFIG_PATH value with this form.

Copy link
Contributor Author

@wagavulin wagavulin Sep 18, 2017

Choose a reason for hiding this comment

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

OK.

c_glib/README.md Outdated
Copy link
Member

@kou kou Sep 17, 2017

Choose a reason for hiding this comment

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

ditto.

Copy link
Contributor Author

@wagavulin wagavulin Sep 18, 2017

Choose a reason for hiding this comment

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

OK.

c_glib/README.md Outdated
Copy link
Member

@kou kou Sep 17, 2017

Choose a reason for hiding this comment

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

Or we can use --with-arrow-cpp-build-type and --with-arrow-cpp-build-dir configure options.
It's better that description of them are included in this pull request but it's not required.

Copy link
Member

@kou kou Sep 17, 2017

Choose a reason for hiding this comment

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

#1100 and https://issues.apache.org/jira/browse/ARROW-1537 may be useful information for macOS.

Copy link
Contributor Author

@wagavulin wagavulin Sep 18, 2017

Choose a reason for hiding this comment

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

Thank you for useful advice. However I think such an advanced setting is not necessary for this "common build failures" section. It assumes that Arrow C++ is installed in accordance with cpp/README.md.

@wesm
Copy link
Member

wesm commented Sep 17, 2017

This should rebase cleanly in apache/master (I rebased master on the release branch and now pushed)

@wesm
Copy link
Member

wesm commented Sep 18, 2017

Please also remove the brackets from "[ARROW-1464]:"

@wagavulin
Add "Common build problems" section in the README.md of c_glib
b6c5274 
Add some detailed explanation of common build problems especially on macOS because it requires some tweaks.
@wagavulin wagavulin changed the title [ARROW-1464]: [GLib] Add "Common build problems" section into the README.md of c_glib ARROW-1464: [GLib] Add "Common build problems" section into the README.md of c_glib Sep 18, 2017
wagavulin
wagavulin commented Sep 18, 2017
View reviewed changes
Copy link
Contributor Author

@wagavulin wagavulin left a comment

Choose a reason for hiding this comment

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

Thank you for your comment. I will try to improve the explanation of brew link issue.

c_glib/README.md Outdated
Copy link
Contributor Author

@wagavulin wagavulin Sep 18, 2017

Choose a reason for hiding this comment

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

OK.

c_glib/README.md Outdated
Copy link
Contributor Author

@wagavulin wagavulin Sep 18, 2017

Choose a reason for hiding this comment

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

OK.

c_glib/README.md Outdated
Copy link
Contributor Author

@wagavulin wagavulin Sep 18, 2017

Choose a reason for hiding this comment

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

OK

c_glib/README.md Outdated
Copy link
Contributor Author

@wagavulin wagavulin Sep 18, 2017

Choose a reason for hiding this comment

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

No. Warning: autoconf-archive 2017.03.21 is already installed, it's just not linked. ... is a warning message of brew install autconf-archive. And Linking /usr/local/Cellar/autoconf-archive/2017.03.21... is an error message of brew link autoconf-archive. So "following" is correct. But I think this paragraph may confusing, so I will rewrite.

c_glib/README.md Outdated
Copy link
Contributor Author

@wagavulin wagavulin Sep 18, 2017

Choose a reason for hiding this comment

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

OK.

c_glib/README.md Outdated
Copy link
Contributor Author

@wagavulin wagavulin Sep 18, 2017

Choose a reason for hiding this comment

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

OK.

c_glib/README.md Outdated
Copy link
Contributor Author

@wagavulin wagavulin Sep 18, 2017

Choose a reason for hiding this comment

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

OK.

c_glib/README.md Outdated
Copy link
Contributor Author

@wagavulin wagavulin Sep 18, 2017

Choose a reason for hiding this comment

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

OK.

c_glib/README.md Outdated
Copy link
Contributor Author

@wagavulin wagavulin Sep 18, 2017

Choose a reason for hiding this comment

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

Thank you for useful advice. However I think such an advanced setting is not necessary for this "common build failures" section. It assumes that Arrow C++ is installed in accordance with cpp/README.md.

@wagavulin
Improve format and the explanation of installing/linking autoconf arc…
9b65542 
…hive on macOS.

Fixed format issues:

* Use '%' for shell prompt.
* Use `console` for console output.
* Add empty lines under header lines.
@wagavulin
Copy link
Contributor Author

wagavulin commented Sep 18, 2017

I have updated based on the comments. Please check.

@asfgit asfgit closed this in 63e7966 Sep 18, 2017
@kou
Copy link
Member

kou commented Sep 18, 2017

Thanks!

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

Reviewers

@kou kou kou requested changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@wagavulin @wesm @kou