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-2894: [Glib] Adjust tests to format refactor #2303

Closed
wants to merge 3 commits into from

Conversation

xhochy
Copy link
Member

@xhochy xhochy commented Jul 22, 2018

cc @kou

I I adjusted the tests to pass. Sadly I had to realise that some of the outputs still don't look pretty. I will change this in a follow-up PR.

There were some issues for me to build Glib in the same environment as the conda one described in https://arrow.apache.org/docs/python/development.html#developing-on-linux-and-macos.

  • my configure command was
./configure PKG_CONFIG_PATH=$(brew --prefix libffi)/lib/pkgconfig:$CONDA_PREFIX/lib/pkgconfig/:$PKG_CONFIG_PATH --prefix=`pwd`/dist
  • I had to run cp $CONDA_PREFIX/lib/libarrow* arrow-glib/.libs/ as otherwise the GISCAN step would not suceed. Even setting LD_LIBRARY_PATH did not help
  • To run the tests I also needed to add GI.prepend_typelib_path('dist/lib/girepository-1.0/') to c_glib/test/run-test.rb. Otherwise GI.load("Arrow") did not find the necessary definitions.

@xhochy xhochy requested a review from kou July 22, 2018 19:07
-- is_valid: all not null
-- dictionary: [1, 3, -1, -3]
-- indices: [0, 1, 0, 2, 3, 2]
-- dictionary:
Copy link
Member

Choose a reason for hiding this comment

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

Is is_valid intentionally missing here?

Copy link
Member

Choose a reason for hiding this comment

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

It seems that new pretty print implementation doesn't show is_valid information.
Can you open a JIRA ticket to discuss what should we do for it?

Copy link
Member Author

Choose a reason for hiding this comment

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

is_valid is a duplicate information. We also show nulls in the indices now, before the refactor we have shown undefined values instead there.

Copy link
Member

Choose a reason for hiding this comment

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

I see. It makes sense.


-- is_valid:
all not null
-- child 0 type: int32 values: [
Copy link
Member

@kszucs kszucs Jul 22, 2018

Choose a reason for hiding this comment

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

I know You're planning to implement it in a follow-up PR, just suggesting that something like this would be more readable (without type and values):

-- child 0: int32 [
      1,
      2
   ]

Copy link
Member Author

Choose a reason for hiding this comment

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

I will take up your suggestion and go for


-- child 0: int32 
  [
      1,
      2
   ]

The line break before [ is mainly due to the fact that makes the array printing code simpler in a first round. We don't need to take the context in consideration then. If that really looks better, we can have a closer look afterwards. My first objective is to iron out any strong misaligments before the release.

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.

+1
Thanks for working on it. :-)
There are more works for Ruby. But I'll push some commits for it.

-- is_valid: all not null
-- dictionary: [1, 3, -1, -3]
-- indices: [0, 1, 0, 2, 3, 2]
-- dictionary:
Copy link
Member

Choose a reason for hiding this comment

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

It seems that new pretty print implementation doesn't show is_valid information.
Can you open a JIRA ticket to discuss what should we do for it?

@kou
Copy link
Member

kou commented Jul 23, 2018

@kou
Copy link
Member

kou commented Jul 23, 2018

I had to run cp $CONDA_PREFIX/lib/libarrow* arrow-glib/.libs/ as otherwise the GISCAN step would not suceed. Even setting LD_LIBRARY_PATH did not help

I could reproduce this.
I can fix it by setting the following LD_LIBRARY_PATH:

% LD_LIBRARY_PATH=$CONDA_PREFIX/lib make

To run the tests I also needed to add GI.prepend_typelib_path('dist/lib/girepository-1.0/') to c_glib/test/run-test.rb. Otherwise GI.load("Arrow") did not find the necessary definitions.

I could also reproduce this.
We need exports:

diff --git a/c_glib/test/run-test.sh b/c_glib/test/run-test.sh
index d563e858..9abd0354 100755
--- a/c_glib/test/run-test.sh
+++ b/c_glib/test/run-test.sh
@@ -33,6 +33,7 @@ for module in ${modules}; do
     fi
   fi
 done
+export LD_LIBRARY_PATH
 
 if [ -f "Makefile" -a "${NO_MAKE}" != "yes" ]; then
   make -j8 > /dev/null || exit $?
@@ -49,5 +50,6 @@ for module in ${modules}; do
     GI_TYPELIB_PATH="${module_typelib_dir}:${GI_TYPELIB_PATH}"
   fi
 done
+export GI_TYPELIB_PATH
 
 ${GDB} ruby ${test_dir}/run-test.rb "$@"

I could run tests by the following with the above change:

%  LD_LIBRARY_PATH=$CONDA_PREFIX/lib test/run-test.sh 

I'll send a pull request for the exports.

@kou
Copy link
Member

kou commented Jul 23, 2018

CI is green. I'll merge this.

@kou kou closed this in a8ec080 Jul 23, 2018
@xhochy xhochy deleted the ARROW-2894 branch July 23, 2018 05:00
xhochy pushed a commit that referenced this pull request Jul 23, 2018
If we don't call export, c_glib/test/run-test.rb can't use
LD_LIBRARY_PATH and GI_TYPELIB_PATH variables.

See also #2303 (comment)

Author: Kouhei Sutou <kou@clear-code.com>

Closes #2305 from kou/glib-add-missing-export and squashes the following commits:

99f90f6 <Kouhei Sutou>  Add missing exports
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.

None yet

3 participants