Skip to content

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

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

Merged
merged 6 commits into from
Nov 22, 2021

Conversation

erlend-aasland
Copy link
Contributor

@erlend-aasland erlend-aasland commented Nov 19, 2021

@erlend-aasland
Copy link
Contributor Author

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

PoC: Apply to SQLite check.
@erlend-aasland
Copy link
Contributor Author

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

@erlend-aasland
Copy link
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)

configure.ac Outdated
Comment on lines 35 to 38
[_SAVE_VAR([CFLAGS])]
[_SAVE_VAR([CPPFLAGS])]
[_SAVE_VAR([LDFLAGS])]
[_SAVE_VAR([LIBS])]
Copy link
Member

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

Copy link
Contributor Author

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.

Copy link
Contributor Author

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?

@erlend-aasland erlend-aasland requested a review from tiran November 21, 2021 16:35
Copy link
Member

@tiran tiran left a comment

Choose a reason for hiding this comment

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

Excellent!

@tiran 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 tiran merged commit db2277a into python:main Nov 22, 2021
@erlend-aasland 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants