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

BUG: use internal linkage (static variables) in move.c #24113

Merged

Conversation

Projects
None yet
3 participants
@llllllllll
Copy link
Contributor

commented Dec 5, 2018

move.c declared global variables without explicitly declaring them static, so
they had global visibility. This meant that if you linked against a shared
object that provided the same symbols, the wrong data would be read. In
particular: linking to libgtk, which provides the symbol 'methods', would cause
an import error of pandas.util._move.

  • closes #19706
  • tests added / passed
  • passes git diff upstream/master -u -- "*.py" | flake8 --diff
  • whatsnew entry
BUG: use internal linkage (static variables) in move.c
move.c declared global variables without explicitly declaring them static, so
they had global visibility. This meant that if you linked against a shared
object that provided the same symbols, the wrong data would be read. In
particular: linking to libgtk, which provides the symbol 'methods', would cause
an import error of pandas.util._move.
@codecov

This comment has been minimized.

Copy link

commented Dec 5, 2018

Codecov Report

Merging #24113 into master will decrease coverage by <.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #24113      +/-   ##
==========================================
- Coverage    92.2%    92.2%   -0.01%     
==========================================
  Files         162      162              
  Lines       51714    51714              
==========================================
- Hits        47682    47681       -1     
- Misses       4032     4033       +1
Flag Coverage Δ
#multiple 90.6% <ø> (ø) ⬆️
#single 43.01% <ø> (-0.01%) ⬇️
Impacted Files Coverage Δ
pandas/io/json/json.py 92.61% <0%> (-0.48%) ⬇️
pandas/util/testing.py 87.51% <0%> (+0.09%) ⬆️

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 9f2c716...c88d9ea. Read the comment docs.

@codecov

This comment has been minimized.

Copy link

commented Dec 5, 2018

Codecov Report

Merging #24113 into master will decrease coverage by <.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #24113      +/-   ##
==========================================
- Coverage    92.2%    92.2%   -0.01%     
==========================================
  Files         162      162              
  Lines       51701    51701              
==========================================
- Hits        47672    47671       -1     
- Misses       4029     4030       +1
Flag Coverage Δ
#multiple 90.6% <ø> (ø) ⬆️
#single 43.02% <ø> (-0.01%) ⬇️
Impacted Files Coverage Δ
pandas/io/json/json.py 92.61% <0%> (-0.48%) ⬇️
pandas/util/testing.py 87.51% <0%> (+0.09%) ⬆️

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 b78aa8d...8b62a41. Read the comment docs.

@llllllllll

This comment has been minimized.

Copy link
Contributor Author

commented Dec 5, 2018

unclear how/if we want to test this case. Also, do you want a whatsnew for a (small) bugfix like this?

@jreback

This comment has been minimized.

Copy link
Contributor

commented Dec 5, 2018

thanks @llllllllll this looks fine. can you add a whatsnew note, ping on green.

@jreback jreback added Bug Compat labels Dec 5, 2018

@jreback jreback added this to the 0.24.0 milestone Dec 5, 2018

@jreback
Copy link
Contributor

left a comment

minor point. ping on green.

@@ -1262,6 +1262,7 @@ Categorical
- In meth:`Series.unstack`, specifying a ``fill_value`` not present in the categories now raises a ``TypeError`` rather than ignoring the ``fill_value`` (:issue:`23284`)
- Bug when resampling :meth:`Dataframe.resample()` and aggregating on categorical data, the categorical dtype was getting lost. (:issue:`23227`)
- Bug in many methods of the ``.str``-accessor, which always failed on calling the ``CategoricalIndex.str`` constructor (:issue:`23555`, :issue:`23556`)
- Bug in :mod:`pandas.utils._move` where C variables were declared with external linkage causing import errors if certain other C libraries were imported before Pandas. (:issue:`24113`)

This comment has been minimized.

Copy link
@jreback

jreback Dec 5, 2018

Contributor

can you move to the other section, and remove the :mod:`pandas.utils._move`

This comment has been minimized.

Copy link
@llllllllll

llllllllll Dec 5, 2018

Author Contributor

just remove the link, or don't mention which module the error was in?

This comment has been minimized.

Copy link
@jreback

jreback Dec 5, 2018

Contributor

just remove the module, i think the statement about c libraries is ok (but move it to the 'other' section, at the bottom)

@llllllllll llllllllll force-pushed the llllllllll:internal-linkage-in-move branch from db17d85 to 8b62a41 Dec 6, 2018

@llllllllll

This comment has been minimized.

Copy link
Contributor Author

commented Dec 6, 2018

I moved the whatsnew entry to other and removed the link. Sorry about putting it in "categorical" before, I just searched for "bugs" quickly. Tests are now passing. cc @jreback

@TomAugspurger TomAugspurger merged commit 03134cb into pandas-dev:master Dec 6, 2018

3 checks passed

ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
pandas-dev.pandas Build #20181206.43 succeeded
Details
@TomAugspurger

This comment has been minimized.

Copy link
Contributor

commented Dec 6, 2018

Thanks!

Pingviinituutti added a commit to Pingviinituutti/pandas that referenced this pull request Feb 28, 2019

BUG: use internal linkage (static variables) in move.c (pandas-dev#24113
)

* BUG: use internal linkage (static variables) in move.c

move.c declared global variables without explicitly declaring them static, so
they had global visibility. This meant that if you linked against a shared
object that provided the same symbols, the wrong data would be read. In
particular: linking to libgtk, which provides the symbol 'methods', would cause
an import error of pandas.util._move.

* DOC: whatsnew entry for pandas.util._move static variables

Pingviinituutti added a commit to Pingviinituutti/pandas that referenced this pull request Feb 28, 2019

BUG: use internal linkage (static variables) in move.c (pandas-dev#24113
)

* BUG: use internal linkage (static variables) in move.c

move.c declared global variables without explicitly declaring them static, so
they had global visibility. This meant that if you linked against a shared
object that provided the same symbols, the wrong data would be read. In
particular: linking to libgtk, which provides the symbol 'methods', would cause
an import error of pandas.util._move.

* DOC: whatsnew entry for pandas.util._move static variables
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.