GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — bpo-45723: Add helpers for save/restore env (GH-29637) by erlend-aasland · Pull Request #29637 · python/cpython

Conversation

erlend-aasland commented Nov 19, 2021 •

edited by bedevere-bot

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Contributor

https://bugs.python.org/issue45723

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

tiran reacted with eyes emoji

All reactions

👀 1 reaction

the-knights-who-say-ni added the CLA signed label

Nov 19, 2021

bedevere-bot added the awaiting review label

Nov 19, 2021

erlend-aasland requested a review from tiran

November 19, 2021 12:33

erlend-aasland commented Nov 19, 2021

Copy link

Copy Markdown

Contributor Author

@tiran: there might be a smoother way to do this, but this should do the trick.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

erlend-aasland mentioned this pull request

Nov 19, 2021

bpo-45774: Autoconfiscate SQLite detection (GH-29507) #29507

Merged


          bpo-45723: Add helpers for save/restore env

PoC: Apply to SQLite check.

erlend-aasland force-pushed the ac-save-env branch from 8316e5f to 6579627 Compare

November 19, 2021 14:27

Erlend E. Aasland added 2 commits

November 19, 2021 15:38


          Relocate comments

cf84557


          Add NEWS

37c02b2

erlend-aasland commented Nov 19, 2021

Copy link

Copy Markdown

Contributor Author

Another option is to patch the built-in macros, but I prefer this explicit approach.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

erlend-aasland commented Nov 19, 2021

Copy link

Copy Markdown

Contributor Author

Possible improvement: minimise future configure diffs further, for improved review experience.

Proposed patch

diff --git a/configure.ac b/configure.ac
index 02463ae717..90507e206b 100644
--- a/configure.ac
+++ b/configure.ac
@@ -32,21 +32,21 @@ dnl - WITH_SAVE_ENV([SCRIPT])  Runs SCRIPT wrapped with SAVE_ENV/RESTORE_ENV
 AC_DEFUN([_SAVE_VAR], [AS_VAR_COPY([save_][$1], [$1])])dnl
 AC_DEFUN([_RESTORE_VAR], [AS_VAR_COPY([$1], [save_][$1])])dnl
 AC_DEFUN([SAVE_ENV],
-  [_SAVE_VAR([CFLAGS])]
-  [_SAVE_VAR([CPPFLAGS])]
-  [_SAVE_VAR([LDFLAGS])]
-  [_SAVE_VAR([LIBS])]
+[_SAVE_VAR([CFLAGS])]
+[_SAVE_VAR([CPPFLAGS])]
+[_SAVE_VAR([LDFLAGS])]
+[_SAVE_VAR([LIBS])]
 )dnl
 AC_DEFUN([RESTORE_ENV],
-  [_RESTORE_VAR([CFLAGS])]
-  [_RESTORE_VAR([CPPFLAGS])]
-  [_RESTORE_VAR([LDFLAGS])]
-  [_RESTORE_VAR([LIBS])]
+[_RESTORE_VAR([CFLAGS])]
+[_RESTORE_VAR([CPPFLAGS])]
+[_RESTORE_VAR([LDFLAGS])]
+[_RESTORE_VAR([LIBS])]
 )dnl
 AC_DEFUN([WITH_SAVE_ENV],
-  [SAVE_ENV]
-  [$1]
-  [RESTORE_ENV]
+[SAVE_ENV]
+m4_strip([$1])
+[RESTORE_ENV]
 )dnl
 
 AC_SUBST(BASECPPFLAGS)

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.


          Fix reST formatting in NEWS entry

c81a3dc

tiran reviewed

Nov 19, 2021

View reviewed changes

Comment thread

configure.ac Outdated

Comment on lines +35 to +38

		[_SAVE_VAR([CFLAGS])]
		[_SAVE_VAR([CPPFLAGS])]
		[_SAVE_VAR([LDFLAGS])]
		[_SAVE_VAR([LIBS])]

tiran Nov 19, 2021

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

It looks like the indention adds some extra space to the output:

save_CFLAGS=$CFLAGS
  save_CPPFLAGS=$CPPFLAGS
  save_LDFLAGS=$LDFLAGS
  save_LIBS=$LIBS

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

erlend-aasland Nov 19, 2021

Copy link

Copy Markdown

Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Yeah. We can tweak the new macros to further minimise configure diffs. I think that could be worth it, as it would be easier to review future PRs.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

tiran reacted with thumbs up emoji

All reactions

👍 1 reaction

erlend-aasland Nov 19, 2021

Copy link

Copy Markdown

Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Does it look better with dcc2ba9 applied?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Erlend E. Aasland added 2 commits

November 19, 2021 18:59


          Try to minimise configure diffs

dcc2ba9


          Merge branch 'main' into ac-save-env

06a6bdf

erlend-aasland requested a review from tiran

November 21, 2021 16:35

tiran approved these changes

Nov 22, 2021

View reviewed changes

tiran left a comment

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Excellent!

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

erlend-aasland reacted with hooray emoji

All reactions

🎉 1 reaction

bedevere-bot added awaiting merge and removed awaiting review labels

Nov 22, 2021

tiran changed the title ~~bpo-45723: Add helpers for save/restore env~~ bpo-45723: Add helpers for save/restore env (GH-29637)

Nov 22, 2021

tiran merged commit db2277a into python:main

Nov 22, 2021

bedevere-bot removed the awaiting merge label

Nov 22, 2021

erlend-aasland deleted the ac-save-env branch

November 22, 2021 08:17

remykarem pushed a commit to remykarem/cpython that referenced this pull request

Dec 7, 2021


          bpo-45723: Add helpers for save/restore env (pythonGH-29637)

949a0a0

tiran mentioned this pull request

Apr 22, 2022

Improve and simplify configure.ac checks #89886

Closed

Reviewers

1 more reviewer

tiran

tiran approved these changes

Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

4 participants

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.

GeezerVison

Conversation

erlend-aasland commented Nov 19, 2021 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

erlend-aasland commented Nov 19, 2021

Uh oh!

erlend-aasland commented Nov 19, 2021

Uh oh!

erlend-aasland commented Nov 19, 2021

Uh oh!

tiran Nov 19, 2021

Choose a reason for hiding this comment

Uh oh!

erlend-aasland Nov 19, 2021

Choose a reason for hiding this comment

Uh oh!

erlend-aasland Nov 19, 2021

Choose a reason for hiding this comment

Uh oh!

tiran left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

erlend-aasland commented Nov 19, 2021 •

edited by bedevere-bot

Loading

Uh oh!

There was an error while loading. Please reload this page.