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

MAINT: small simplification of meson.build following best practices #54737

Merged
merged 3 commits into from
Aug 28, 2023
Merged

MAINT: small simplification of meson.build following best practices #54737

merged 3 commits into from
Aug 28, 2023

Conversation

dnicolodi
Copy link
Contributor

Just a couple of things I observed having a quick look at meson.build.

@dnicolodi dnicolodi requested a review from WillAyd as a code owner August 24, 2023 20:27
@WillAyd
Copy link
Member

WillAyd commented Aug 24, 2023

@lithomas1

@mroeschke mroeschke added the Build Library building on various platforms label Aug 24, 2023
lithomas1
lithomas1 reviewed Aug 25, 2023
View reviewed changes
pandas/_libs/window/meson.build
@@ -3,7 +3,6 @@ py.extension_module(
['aggregations.pyx'],
cython_args: ['-X always_allow_keywords=true'],
include_directories: [inc_np, inc_pd],
dependencies: [py_dep],
Copy link
Member

Choose a reason for hiding this comment

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

Can you do the same for pandas/_libs/meson.build and pandas/_libs/tslibs/meson.build?

Copy link
Contributor Author

@dnicolodi dnicolodi Aug 26, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Unless I'm misunderstanding the request, these other meson.build files do not contain the problematic construct.

@WillAyd
Copy link
Member

WillAyd commented Aug 26, 2023

Another cleanup I noticed in the context of the optimization discussion we had is that every extension right now picks up CFLAGS and CPPFLAGS, yet we only have one extension module that actually uses c++ (window/aggregations.pyx)

Would be nice if we didn't mix those up (doesn't need to be done in this PR)

@lithomas1 lithomas1 added this to the 2.1 milestone Aug 28, 2023
@lithomas1 lithomas1 merged commit ca42994 into pandas-dev:main Aug 28, 2023
@lithomas1
Copy link
Member

thanks @dnicolodi

@meeseeksmachine meeseeksmachine mentioned this pull request Aug 28, 2023
Merged
meeseeksmachine pushed a commit to meeseeksmachine/pandas that referenced this pull request Aug 28, 2023
@dnicolodi @meeseeksmachine
Backport PR pandas-dev#54737: MAINT: small simplification of meson.bu…
1838776 
…ild following best practices
@phofl
Copy link
Member

phofl commented Aug 28, 2023

This broke the windows builds

phofl added a commit that referenced this pull request Aug 28, 2023
@phofl
Revert "MAINT: small simplification of meson.build following best pra…
93a5b8f 
…ctices (#54737)"

This reverts commit ca42994.
@phofl phofl mentioned this pull request Aug 28, 2023
Merged
@dnicolodi
Copy link
Contributor Author

This broke the windows builds

Didn't CI pass on these changes? How does the build break?

@phofl
Copy link
Member

phofl commented Aug 28, 2023

The windows builds didn't pass here either

@dnicolodi
Copy link
Contributor Author

I see. I didn't notice the problem before this PR was merged. Sorry for the trouble.

The issue is this:

..\..\meson.build:2:0: ERROR: Command `C:\hostedtoolcache\windows\Python\3.9.13\x64\python3.EXE generate_version.py --print` failed with status 1.

This is the command that generates the version string in the project() call. I changed that from using python to python3. I don't understand why that fails though. It seems that python3 is found.

@dnicolodi
Copy link
Contributor Author

If you need a quick resolution, can you please revert just the last commit in this PR, and not the whole thing?

@dnicolodi
Copy link
Contributor Author

Curiously, the python-dev (windows-latest) job passes. While the other Windows build fail. I wonder if it is a build environment issue. It would be interesting to see the content of the meson log in D:\a\pandas\pandas\build\cp311\meson-logs\meson-log.txt

@lithomas1
Copy link
Member

Ah sorry, didn't realize it was the Windows not the npdev that was failing.

phofl added a commit that referenced this pull request Aug 28, 2023
@phofl @lithomas1
Revert "MAINT: small simplification of meson.build following best pra…
0a23624 
…ctices" (#54802)

* Revert "MAINT: small simplification of meson.build following best practices (#54737)"

This reverts commit ca42994.

* Update meson.build

* Update meson.build

---------

Co-authored-by: Thomas Li <47963215+lithomas1@users.noreply.github.com>
phofl added a commit that referenced this pull request Aug 28, 2023
@meeseeksmachine @dnicolodi @phofl
Backport PR #54737 on branch 2.1.x (MAINT: small simplification of me…
4855f54 
…son.build following best practices) (#54799)

* Backport PR #54737: MAINT: small simplification of meson.build following best practices

* Update meson.build

---------

Co-authored-by: Daniele Nicolodi <daniele@grinta.net>
Co-authored-by: Patrick Hoefler <61934744+phofl@users.noreply.github.com>
mroeschke pushed a commit to mroeschke/pandas that referenced this pull request Sep 11, 2023
@dnicolodi @mroeschke
MAINT: small simplification of meson.build following best practices (p…
4768051 
…andas-dev#54737)

* MAINT: small simplification of meson.build following best practices

* MAINT: remove comment about resolved issue

* BUG: fix build with default Homebrew Python setup

Homebrew does not install a python link, just python3.
mroeschke pushed a commit to mroeschke/pandas that referenced this pull request Sep 11, 2023
@phofl @lithomas1 @mroeschke
Revert "MAINT: small simplification of meson.build following best pra…
54b47e2 
…ctices" (pandas-dev#54802)

* Revert "MAINT: small simplification of meson.build following best practices (pandas-dev#54737)"

This reverts commit ca42994.

* Update meson.build

* Update meson.build

---------

Co-authored-by: Thomas Li <47963215+lithomas1@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lithomas1 lithomas1 lithomas1 left review comments

@WillAyd WillAyd Awaiting requested review from WillAyd WillAyd is a code owner

Assignees
No one assigned
Labels
Build Library building on various platforms
Projects
None yet
Milestone
2.1
Development

Successfully merging this pull request may close these issues.
5 participants
@dnicolodi @WillAyd @lithomas1 @phofl @mroeschke