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.

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

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

Closed
wants to merge 2 commits into from
Closed

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

wants to merge 2 commits into from

Conversation

wagavulin
Copy link
Contributor

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
* Check whether `autoconf-archive` is installed.
* [macOS] Run `brew install autoconf-archive` again. If it shows like the following message, run `brew link autoconf-archive`.

```text
Copy link
Member

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

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
You can use `brew link autoconf-archive` to link this version.
```

Some packages (e.g. `gnome-common`) conflict with `autoconf-archive`. If you see like the following message, run `brew unlink <pkgname>`, then run `brew link autoconf-archive` again.
Copy link
Member

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

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

Some packages (e.g. `gnome-common`) conflict with `autoconf-archive`. If you see like the following message, run `brew unlink <pkgname>`, then run `brew link autoconf-archive` again.

```text
Copy link
Member

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

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
@@ -197,3 +197,37 @@ Now, you can run unit tests by the followings:
% cd c_glib
% test/run-test.sh
```

## Common build problems
Copy link
Member

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

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
* [macOS] Run `brew install autoconf-archive` again. If it shows like the following message, run `brew link autoconf-archive`.

```text
$ brew install autoconf-archive
Copy link
Member

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

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
Target /usr/local/share/aclocal/ax_check_enable_debug.m4
is a symlink belonging to gnome-common. You can unlink it:
brew unlink gnome-common
```
Copy link
Member

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

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
gobject-introspection requires libffi, and it's automatically installed with gobject-introspection. However it can't be found because it's [keg-only](https://docs.brew.sh/FAQ.html#what-does-keg-only-mean). You need to set `PKG_CONFIG_PATH` when executing configure.

```text
% PKG_CONFIG_PATH=$(brew --prefix libffi)/lib/pkgconfig ./configure
Copy link
Member

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

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
% PKG_CONFIG_PATH=$(brew --prefix libffi)/lib/pkgconfig ./configure
```

### build failed - /usr/bin/ld: cannot find -larrow
Copy link
Member

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

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
```

### build failed - /usr/bin/ld: cannot find -larrow
Arrow C++ must be installed to build Arrow GLib. Run `make install` on Arrow C++ build directory. In addtion, on linux, you may need to run `sudo ldconfig`.
Copy link
Member

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

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

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
@@ -197,3 +197,37 @@ Now, you can run unit tests by the followings:
% cd c_glib
% test/run-test.sh
```

## Common build problems
Copy link
Contributor Author

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
* Check whether `autoconf-archive` is installed.
* [macOS] Run `brew install autoconf-archive` again. If it shows like the following message, run `brew link autoconf-archive`.

```text
Copy link
Contributor Author

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
* [macOS] Run `brew install autoconf-archive` again. If it shows like the following message, run `brew link autoconf-archive`.

```text
$ brew install autoconf-archive
Copy link
Contributor Author

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
You can use `brew link autoconf-archive` to link this version.
```

Some packages (e.g. `gnome-common`) conflict with `autoconf-archive`. If you see like the following message, run `brew unlink <pkgname>`, then run `brew link autoconf-archive` again.
Copy link
Contributor Author

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

Some packages (e.g. `gnome-common`) conflict with `autoconf-archive`. If you see like the following message, run `brew unlink <pkgname>`, then run `brew link autoconf-archive` again.

```text
Copy link
Contributor Author

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
Target /usr/local/share/aclocal/ax_check_enable_debug.m4
is a symlink belonging to gnome-common. You can unlink it:
brew unlink gnome-common
```
Copy link
Contributor Author

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
gobject-introspection requires libffi, and it's automatically installed with gobject-introspection. However it can't be found because it's [keg-only](https://docs.brew.sh/FAQ.html#what-does-keg-only-mean). You need to set `PKG_CONFIG_PATH` when executing configure.

```text
% PKG_CONFIG_PATH=$(brew --prefix libffi)/lib/pkgconfig ./configure
Copy link
Contributor Author

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
% PKG_CONFIG_PATH=$(brew --prefix libffi)/lib/pkgconfig ./configure
```

### build failed - /usr/bin/ld: cannot find -larrow
Copy link
Contributor Author

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
```

### build failed - /usr/bin/ld: cannot find -larrow
Arrow C++ must be installed to build Arrow GLib. Run `make install` on Arrow C++ build directory. In addtion, on linux, you may need to run `sudo ldconfig`.
Copy link
Contributor Author

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

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.

None yet
3 participants
@wagavulin @wesm @kou