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

Fix build warning for use of strdup in ultrajson #48369

Merged
merged 1 commit into from
Sep 20, 2022

Conversation

rgommers
Copy link
Contributor

@rgommers rgommers commented Sep 2, 2022

When building with -std=c99 (I was having a look at @lithomas1's meson-poc branch) on Linux this gives a warning:

[2/3] Compiling C object pandas/_libs/json.cpython-310-x86_64-linux-gnu.so.p/src_ujson_lib_ultrajsonenc.c.o
../pandas/_libs/src/ujson/lib/ultrajsonenc.c: In function 'JSON_EncodeObject':
../pandas/_libs/src/ujson/lib/ultrajsonenc.c:1180:18: warning: implicit declaration of function 'strdup'; did you mean 'strcmp'? [-Wimplicit-function-declaration]
 1180 |         locale = strdup(locale);
      |                  ^~~~~~
      |                  strcmp
../pandas/_libs/src/ujson/lib/ultrajsonenc.c:1180:16: warning: assignment to 'char *' from 'int' makes pointer from integer without a cast [-Wint-conversion]
 1180 |         locale = strdup(locale);

And for the MSVC builds in CI the warnings look like:

building 'pandas._libs.json' extension
creating build\temp.win-amd64-cpython-310\Release\pandas\_libs\src\ujson
creating build\temp.win-amd64-cpython-310\Release\pandas\_libs\src\ujson\lib
creating build\temp.win-amd64-cpython-310\Release\pandas\_libs\src\ujson\python
"C:\Program Files\Microsoft Visual Studio\2022\Enterprise\VC\Tools\MSVC\14.33.31629\bin\HostX86\x64\cl.exe" /c /nologo /O2 /W3 /GL /DNDEBUG /MD -DNPY_NO_DEPRECATED_API=0 -Ipandas/_libs/src/ujson/python -Ipandas/_libs/src/ujson/lib -Ipandas/_libs/src/datetime -IC:\Users\runneradmin\micromamba\envs\test\lib\site-packages\numpy\core\include -IC:\Users\runneradmin\micromamba\envs\test\include -IC:\Users\runneradmin\micromamba\envs\test\Include "-IC:\Program Files\Microsoft Visual Studio\2022\Enterprise\VC\Tools\MSVC\14.33.31629\include" "-IC:\Program Files\Microsoft Visual Studio\2022\Enterprise\VC\Tools\MSVC\14.33.31629\ATLMFC\include" "-IC:\Program Files\Microsoft Visual Studio\2022\Enterprise\VC\Auxiliary\VS\include" "-IC:\Program Files (x86)\Windows Kits\10\include\10.0.22621.0\ucrt" "-IC:\Program Files (x86)\Windows Kits\10\\include\10.0.22621.0\\um" "-IC:\Program Files (x86)\Windows Kits\10\\include\10.0.22621.0\\shared" "-IC:\Program Files (x86)\Windows Kits\10\\include\10.0.22621.0\\winrt" "-IC:\Program Files (x86)\Windows Kits\10\\include\10.0.22621.0\\cppwinrt" "-IC:\Program Files (x86)\Windows Kits\NETFXSDK\4.8\include\um" /Tcpandas/_libs/src/ujson/lib/ultrajsondec.c /Fobuild\temp.win-amd64-cpython-310\Release\pandas/_libs/src/ujson/lib/ultrajsondec.obj -D_GNU_SOURCE
ultrajsondec.c
pandas/_libs/src/ujson/lib/ultrajsondec.c(1178): warning C4996: 'strdup': The POSIX name for this item is deprecated. Instead, use the ISO C and C++ conformant name: _strdup. See online help for details.

I considered avoiding the code duplication by putting this block in a shared file, but the two files touched here already have quite a bit of duplicated code, so that's best left alone.

@eli-schwartz
Copy link
Contributor

eli-schwartz commented Sep 2, 2022

On Linux, this should be provided if _XOPEN_SOURCE is defined to at least 500, or _POSIX_C_SOURCE to at least 200809L (when string.h is included).

@rgommers
Copy link
Contributor Author

rgommers commented Sep 2, 2022

On Linux, this should be provided if _XOPEN_SOURCE is defined to at least 500, or _POSIX_C_SOURCE to at least 200809L (when string.h is included).

Thanks @eli-schwartz. That's an alternative indeed, and I did consider it - but not sure if that is better / more portable?

@eli-schwartz
Copy link
Contributor

eli-schwartz commented Sep 2, 2022

It's certainly more portable, it's a POSIX function and that's the POSIX define that is supposed to make it accessible: https://pubs.opengroup.org/onlinepubs/9699919799/functions/V2_chap02.html#tag_15_02

The advantage is that instead of telling the compiler "I pinky promise the system runtime library actually provides this", you tell the system runtime headers to actually provide it. If it still doesn't provide it, then you're probably on Windows, in which case the _MSC_VER workaround is apropos, or else something probably went wrong and you're delaying a compiler diagnostic / error to a linker error.

@gfyoung gfyoung added the Build Library building on various platforms label Sep 2, 2022
@lithomas1 lithomas1 requested a review from WillAyd September 4, 2022 14:46
@WillAyd
Copy link
Member

WillAyd commented Sep 4, 2022

Great thanks for this - always good to clean these up. We already define some portability items in pandas/_libs/src/headers/portable.h - maybe worth placing there instead?

When building with `-std=c99` on Linux this gives a warning:

```
[2/3] Compiling C object pandas/_libs/json.cpython-310-x86_64-linux-gnu.so.p/src_ujson_lib_ultrajsonenc.c.o
../pandas/_libs/src/ujson/lib/ultrajsonenc.c: In function 'JSON_EncodeObject':
../pandas/_libs/src/ujson/lib/ultrajsonenc.c:1180:18: warning: implicit declaration of function 'strdup'; did you mean 'strcmp'? [-Wimplicit-function-declaration]
 1180 |         locale = strdup(locale);
      |                  ^~~~~~
      |                  strcmp
../pandas/_libs/src/ujson/lib/ultrajsonenc.c:1180:16: warning: assignment to 'char *' from 'int' makes pointer from integer without a cast [-Wint-conversion]
 1180 |         locale = strdup(locale);
```

And for the MSVC build in CI the warnings look like:
```
building 'pandas._libs.json' extension
creating build\temp.win-amd64-cpython-310\Release\pandas\_libs\src\ujson
creating build\temp.win-amd64-cpython-310\Release\pandas\_libs\src\ujson\lib
creating build\temp.win-amd64-cpython-310\Release\pandas\_libs\src\ujson\python
"C:\Program Files\Microsoft Visual Studio\2022\Enterprise\VC\Tools\MSVC\14.33.31629\bin\HostX86\x64\cl.exe" /c /nologo /O2 /W3 /GL /DNDEBUG /MD -DNPY_NO_DEPRECATED_API=0 -Ipandas/_libs/src/ujson/python -Ipandas/_libs/src/ujson/lib -Ipandas/_libs/src/datetime -IC:\Users\runneradmin\micromamba\envs\test\lib\site-packages\numpy\core\include -IC:\Users\runneradmin\micromamba\envs\test\include -IC:\Users\runneradmin\micromamba\envs\test\Include "-IC:\Program Files\Microsoft Visual Studio\2022\Enterprise\VC\Tools\MSVC\14.33.31629\include" "-IC:\Program Files\Microsoft Visual Studio\2022\Enterprise\VC\Tools\MSVC\14.33.31629\ATLMFC\include" "-IC:\Program Files\Microsoft Visual Studio\2022\Enterprise\VC\Auxiliary\VS\include" "-IC:\Program Files (x86)\Windows Kits\10\include\10.0.22621.0\ucrt" "-IC:\Program Files (x86)\Windows Kits\10\\include\10.0.22621.0\\um" "-IC:\Program Files (x86)\Windows Kits\10\\include\10.0.22621.0\\shared" "-IC:\Program Files (x86)\Windows Kits\10\\include\10.0.22621.0\\winrt" "-IC:\Program Files (x86)\Windows Kits\10\\include\10.0.22621.0\\cppwinrt" "-IC:\Program Files (x86)\Windows Kits\NETFXSDK\4.8\include\um" /Tcpandas/_libs/src/ujson/lib/ultrajsondec.c /Fobuild\temp.win-amd64-cpython-310\Release\pandas/_libs/src/ujson/lib/ultrajsondec.obj -D_GNU_SOURCE
ultrajsondec.c
pandas/_libs/src/ujson/lib/ultrajsondec.c(1178): warning C4996: 'strdup': The POSIX name for this item is deprecated. Instead, use the ISO C and C++ conformant name: _strdup. See online help for details.
```

Note that on POSIX defining the `_XOPEN_SOURCE` macro to 500 is enough
to make `strdup` available, however on macOS it seems to need 600.
@rgommers
Copy link
Contributor Author

Updated to use _XOPEN_SOURCE (600 rather than 500, which seems necessary with XCode 12 on macOS) and moved to portable.h as suggested.

@lithomas1 lithomas1 added this to the 1.6 milestone Sep 20, 2022
@lithomas1
Copy link
Member

Thanks for the PR.

@WillAyd WillAyd merged commit 9ca7729 into pandas-dev:main Sep 20, 2022
@WillAyd
Copy link
Member

WillAyd commented Sep 20, 2022

Awesome thanks a ton @rgommers

@rgommers rgommers deleted the strdup-warning branch September 20, 2022 14:37
@rgommers
Copy link
Contributor Author

Thanks for the reviews & merge. And yay, my first Pandas PR (long overdue) 🎉

phofl pushed a commit to phofl/pandas that referenced this pull request Sep 22, 2022
mroeschke added a commit that referenced this pull request Sep 26, 2022
…8662)

* BUG: Series.getitem not falling back to positional for bool index

* Update pandas/tests/series/indexing/test_getitem.py

Co-authored-by: Matthew Roeschke <10647082+mroeschke@users.noreply.github.com>

* Fix build warning for use of `strdup` in ultrajson (#48369)

* WEB: Update versions json to fix version switcher in the docs (#48655)

* PERF: join/merge on subset of MultiIndex (#48611)

* DOC: Update documentation for date_range(), bdate_range(), and interval_range() to include timedelta as a possible data type for the freq parameter (#48631)

* Update documentation for date_range(), bdate_range(), and interval_range() to include timedelta as a possible data type for the freq parameter

* Add test case for date_range construction using datetime.timedelta

* TYP: tighten Axis (#48612)

* TYP: tighten Axis

* allow 'rows'

* BUG: Fix metadata propagation in df.corr and df.cov, GH28283 (#48616)

* Add finalize to df.corr and df.cov

* Clean

* TST: add test case for PeriodIndex in HDFStore(GH7796) (#48618)

* TST: add test case for PeriodIndex in HDFStore

* TST: add test case for PeriodIndex in HDFStore

* use pytest.mark.parameterize instead

* Add OpenSSF Scorecards GitHub Action (#48570)

* Create scorecards.yml

* Update scorecards.yml

* Add OpenSSF Scorecards badge to README.md

* Trim whitespace in scorecards.yml

* Skip scorecards.yml on forks

* Fix whitespace

* Pin scorecards.yml dependencies to major versions

* ENH: move an exception and add a prehook to check for exception place… (#48088)

* ENH: move an exception and add a prehook to check for exception placement

* ENH: fix import

* ENH: revert moving error

* ENH: add docstring and fix import for test

* ENH: re-design approach based on feedback

* ENH: update whatsnew rst

* ENH: apply feedback changes

* ENH: refactor to remove exception_warning_list and ignore _version.py

* ENH: remove NotThisMethod from tests and all

* REGR: TextIOWrapper raising an error in read_csv (#48651)

* REGR: TextIOWrapper raising an error in read_csv

* pyupgrade

* do not try to seek on unseekable buffers

* unseekable buffer might also have read ahead

* safer alternative: do not mess with internal/private(?) buffer of TextIOWrapper (effectively applies the shortcut only to files pandas opens)

* Fix scorecard.yml workflow (#48668)

* Set scorecard-action to v2.0.3

scorecard-action does not have a major version tag.

Temporarily disabling github.repository check to ensure action now works.

* Enable github.repository check

* BUG: DatetimeIndex ignoring explicit tz=None (#48659)

* BUG: DatetimeIndex ignoring explicit tz=None

* GH ref

* Corrected pd.merge indicator type hint (#48677)

* Corrected pd.merge indicator type hint

https://pandas.pydata.org/docs/reference/api/pandas.merge.html
It should be "str | bool" instead of just string

* Update merge.py

fixed type hint in merge.py

* Update merge.py

Update indicator type hint in _MergeOperation

* Update merge.py

Added type hint _MergeOperation init

* DOC: Document default value for options.display.max_cols when not running in terminal (#48672)

DOC: Document default value for options.display.max_cols

display.max_cols has a default value of 20 when not running in a terminal
such as Jupyter Notebook

* ENH: DTA/TDA add datetimelike scalar with mismatched reso (#48669)

* ENH: DTA/TDA add datetimelike scalar with mismatched reso

* mypy fixup

* REF: support reso in remaining tslibs helpers (#48661)

* REF: support reso in remaining tslibs helpers

* update setup.py

* PERF: Avoid fragmentation of DataFrame in read_sas (#48603)

* PERF: Avoid fragmentation of DataFrame in read_sas

* Add whatsnew

* Add warning

* DOC: Add deprecation infos to deprecated functions (#48599)

* DOC: Add deprecation infos to deprecated functions

* Add sections

* Fix

* BLD: Build wheels using cibuildwheel (#48283)

* BLD: Build wheels using cibuildwheel

* update from code review

Co-Authored-By: Matthew Roeschke <10647082+mroeschke@users.noreply.github.com>

* fix 3.11 version

* changes from code review

* Update test_wheels.py

* sync run time with pandas-wheels

Co-authored-by: Matthew Roeschke <10647082+mroeschke@users.noreply.github.com>

* REGR: Performance decrease in factorize (#48620)

* TYP: type all arguments with str default values (#48508)

* TYP: type all arguments with str default values

* na_rep: back to str

* na(t)_rep is always a string

* add float for some functions

* and the same for the few float default arguments

* define a few more literal constants

* avoid itertools.cycle mypy error

* revert mistake

* TST: Catch more pyarrow PerformanceWarnings (#48699)

* REGR: to_hdf raising AssertionError with boolean index (#48696)

* REGR: to_hdf raising AssertionError with boolean index

* Add gh ref

* REGR: Regression in DataFrame.loc when setting df with all True indexer (#48711)

* BUG: pivot_table raising for nullable dtype and margins (#48714)

* TST: Address MPL 3.6 deprecation warnings (#48695)

* TST: Address MPL 3.6 deprecation warnings

* Address min build

* missing ()

Co-authored-by: Matthew Roeschke <10647082+mroeschke@users.noreply.github.com>
Co-authored-by: Ralf Gommers <ralf.gommers@gmail.com>
Co-authored-by: Marc Garcia <garcia.marc@gmail.com>
Co-authored-by: Luke Manley <lukemanley@gmail.com>
Co-authored-by: Siddhartha Gandhi <siddhartha.a.gandhi@gmail.com>
Co-authored-by: Torsten Wörtwein <twoertwein@users.noreply.github.com>
Co-authored-by: Xiao Yuan <yuanx749@gmail.com>
Co-authored-by: paradox-lab <57354735+paradox-lab@users.noreply.github.com>
Co-authored-by: Pedro Nacht <15221358+pnacht@users.noreply.github.com>
Co-authored-by: dataxerik <dsshar@gmail.com>
Co-authored-by: jbrockmendel <jbrockmendel@gmail.com>
Co-authored-by: Pablo <48098178+PabloRuizCuevas@users.noreply.github.com>
Co-authored-by: tmoschou <5567550+tmoschou@users.noreply.github.com>
Co-authored-by: Thomas Li <47963215+lithomas1@users.noreply.github.com>
Co-authored-by: Richard Shadrach <45562402+rhshadrach@users.noreply.github.com>
@mroeschke mroeschke modified the milestones: 1.6, 2.0 Oct 13, 2022
noatamir pushed a commit to noatamir/pandas that referenced this pull request Nov 9, 2022
noatamir pushed a commit to noatamir/pandas that referenced this pull request Nov 9, 2022
…ndas-dev#48662)

* BUG: Series.getitem not falling back to positional for bool index

* Update pandas/tests/series/indexing/test_getitem.py

Co-authored-by: Matthew Roeschke <10647082+mroeschke@users.noreply.github.com>

* Fix build warning for use of `strdup` in ultrajson (pandas-dev#48369)

* WEB: Update versions json to fix version switcher in the docs (pandas-dev#48655)

* PERF: join/merge on subset of MultiIndex (pandas-dev#48611)

* DOC: Update documentation for date_range(), bdate_range(), and interval_range() to include timedelta as a possible data type for the freq parameter (pandas-dev#48631)

* Update documentation for date_range(), bdate_range(), and interval_range() to include timedelta as a possible data type for the freq parameter

* Add test case for date_range construction using datetime.timedelta

* TYP: tighten Axis (pandas-dev#48612)

* TYP: tighten Axis

* allow 'rows'

* BUG: Fix metadata propagation in df.corr and df.cov, GH28283 (pandas-dev#48616)

* Add finalize to df.corr and df.cov

* Clean

* TST: add test case for PeriodIndex in HDFStore(GH7796) (pandas-dev#48618)

* TST: add test case for PeriodIndex in HDFStore

* TST: add test case for PeriodIndex in HDFStore

* use pytest.mark.parameterize instead

* Add OpenSSF Scorecards GitHub Action (pandas-dev#48570)

* Create scorecards.yml

* Update scorecards.yml

* Add OpenSSF Scorecards badge to README.md

* Trim whitespace in scorecards.yml

* Skip scorecards.yml on forks

* Fix whitespace

* Pin scorecards.yml dependencies to major versions

* ENH: move an exception and add a prehook to check for exception place… (pandas-dev#48088)

* ENH: move an exception and add a prehook to check for exception placement

* ENH: fix import

* ENH: revert moving error

* ENH: add docstring and fix import for test

* ENH: re-design approach based on feedback

* ENH: update whatsnew rst

* ENH: apply feedback changes

* ENH: refactor to remove exception_warning_list and ignore _version.py

* ENH: remove NotThisMethod from tests and all

* REGR: TextIOWrapper raising an error in read_csv (pandas-dev#48651)

* REGR: TextIOWrapper raising an error in read_csv

* pyupgrade

* do not try to seek on unseekable buffers

* unseekable buffer might also have read ahead

* safer alternative: do not mess with internal/private(?) buffer of TextIOWrapper (effectively applies the shortcut only to files pandas opens)

* Fix scorecard.yml workflow (pandas-dev#48668)

* Set scorecard-action to v2.0.3

scorecard-action does not have a major version tag.

Temporarily disabling github.repository check to ensure action now works.

* Enable github.repository check

* BUG: DatetimeIndex ignoring explicit tz=None (pandas-dev#48659)

* BUG: DatetimeIndex ignoring explicit tz=None

* GH ref

* Corrected pd.merge indicator type hint (pandas-dev#48677)

* Corrected pd.merge indicator type hint

https://pandas.pydata.org/docs/reference/api/pandas.merge.html
It should be "str | bool" instead of just string

* Update merge.py

fixed type hint in merge.py

* Update merge.py

Update indicator type hint in _MergeOperation

* Update merge.py

Added type hint _MergeOperation init

* DOC: Document default value for options.display.max_cols when not running in terminal (pandas-dev#48672)

DOC: Document default value for options.display.max_cols

display.max_cols has a default value of 20 when not running in a terminal
such as Jupyter Notebook

* ENH: DTA/TDA add datetimelike scalar with mismatched reso (pandas-dev#48669)

* ENH: DTA/TDA add datetimelike scalar with mismatched reso

* mypy fixup

* REF: support reso in remaining tslibs helpers (pandas-dev#48661)

* REF: support reso in remaining tslibs helpers

* update setup.py

* PERF: Avoid fragmentation of DataFrame in read_sas (pandas-dev#48603)

* PERF: Avoid fragmentation of DataFrame in read_sas

* Add whatsnew

* Add warning

* DOC: Add deprecation infos to deprecated functions (pandas-dev#48599)

* DOC: Add deprecation infos to deprecated functions

* Add sections

* Fix

* BLD: Build wheels using cibuildwheel (pandas-dev#48283)

* BLD: Build wheels using cibuildwheel

* update from code review

Co-Authored-By: Matthew Roeschke <10647082+mroeschke@users.noreply.github.com>

* fix 3.11 version

* changes from code review

* Update test_wheels.py

* sync run time with pandas-wheels

Co-authored-by: Matthew Roeschke <10647082+mroeschke@users.noreply.github.com>

* REGR: Performance decrease in factorize (pandas-dev#48620)

* TYP: type all arguments with str default values (pandas-dev#48508)

* TYP: type all arguments with str default values

* na_rep: back to str

* na(t)_rep is always a string

* add float for some functions

* and the same for the few float default arguments

* define a few more literal constants

* avoid itertools.cycle mypy error

* revert mistake

* TST: Catch more pyarrow PerformanceWarnings (pandas-dev#48699)

* REGR: to_hdf raising AssertionError with boolean index (pandas-dev#48696)

* REGR: to_hdf raising AssertionError with boolean index

* Add gh ref

* REGR: Regression in DataFrame.loc when setting df with all True indexer (pandas-dev#48711)

* BUG: pivot_table raising for nullable dtype and margins (pandas-dev#48714)

* TST: Address MPL 3.6 deprecation warnings (pandas-dev#48695)

* TST: Address MPL 3.6 deprecation warnings

* Address min build

* missing ()

Co-authored-by: Matthew Roeschke <10647082+mroeschke@users.noreply.github.com>
Co-authored-by: Ralf Gommers <ralf.gommers@gmail.com>
Co-authored-by: Marc Garcia <garcia.marc@gmail.com>
Co-authored-by: Luke Manley <lukemanley@gmail.com>
Co-authored-by: Siddhartha Gandhi <siddhartha.a.gandhi@gmail.com>
Co-authored-by: Torsten Wörtwein <twoertwein@users.noreply.github.com>
Co-authored-by: Xiao Yuan <yuanx749@gmail.com>
Co-authored-by: paradox-lab <57354735+paradox-lab@users.noreply.github.com>
Co-authored-by: Pedro Nacht <15221358+pnacht@users.noreply.github.com>
Co-authored-by: dataxerik <dsshar@gmail.com>
Co-authored-by: jbrockmendel <jbrockmendel@gmail.com>
Co-authored-by: Pablo <48098178+PabloRuizCuevas@users.noreply.github.com>
Co-authored-by: tmoschou <5567550+tmoschou@users.noreply.github.com>
Co-authored-by: Thomas Li <47963215+lithomas1@users.noreply.github.com>
Co-authored-by: Richard Shadrach <45562402+rhshadrach@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Build Library building on various platforms
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants