Skip to content

Conversation

@erlend-aasland
Copy link
Contributor

@erlend-aaslanderlend-aasland commented Feb 21, 2024

@erlend-aasland
Copy link
ContributorAuthor

@mhsmith, would you like to review this?

@erlend-aasland
Copy link
ContributorAuthor

@corona10, feel free to take a look at this, if you have the time.

@erlend-aasland
Copy link
ContributorAuthor

@mhsmith, thanks for the review; I addressed your remarks in 9c3596d. PTAL.

@erlend-aasland

This comment was marked as outdated.

@erlend-aasland
Copy link
ContributorAuthor

Found the culprit; typo on my end! ✅

@erlend-aasland
Copy link
ContributorAuthor

Now, there's one more thing we should add before landing this: support configure cache.

@erlend-aasland
Copy link
ContributorAuthor

Now, there's one more thing we should add before landing this: support configure cache.

I'm not sure we want to automatically generate the cache variable name in this macro. The first argument would need heavy string modifications to end up as a usable identifier. Perhaps it is best to introduce a new param for the cache variable name.

@mhsmith
Copy link
Member

Alternatively, all but one usage of PY_CHECK_CPP could be replaced with AX_CHECK_DEFINE, which has caching already built in.

@erlend-aasland
Copy link
ContributorAuthor

erlend-aasland commented Feb 27, 2024

Alternatively, all but one usage of PY_CHECK_CPP could be replaced with AX_CHECK_DEFINE, which has caching already built in.

That's a good suggestion; I'll have a look in a day or two.

UPDATE: Created alternative PR gh-116016.

@erlend-aasland
Copy link
ContributorAuthor

Alternatively, all but one usage of PY_CHECK_CPP could be replaced with AX_CHECK_DEFINE, which has caching already built in.

For the glibc check; do we really need to worry about glibc < 2.1?

@mhsmith
Copy link
Member

I agree, so #116016 looks like the way to go.

@erlend-aasland
Copy link
ContributorAuthor

Superseded by #116016

@erlend-aaslanderlend-aasland deleted the ac/2.72-egrep-source branch February 28, 2024 13:36
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@erlend-aasland@mhsmith