Skip to content

Conversation

@dimpase
Copy link
Member

@dimpase dimpase commented Jan 6, 2026

fixes a bug in #41333

📝 Checklist

  • The title is concise and informative.
  • The description explains in detail what this PR is about.
  • I have linked a relevant issue or discussion.
  • I have created tests covering the changes.
  • I have updated the documentation and checked the documentation preview.

⌛ Dependencies

@dimpase
Copy link
Member Author

dimpase commented Jan 6, 2026

hmm, no, it doesn't work in macOS (it does work in shell, but does not work when invoked in the script)

  • works now

@github-actions
Copy link

github-actions bot commented Jan 6, 2026

Documentation preview for this PR (built with commit ff27f83; changes) is ready! 🎉
This preview will update shortly after each push to this PR.

@jhpalmieri
Copy link
Member

This works for me. Thanks for investigating and finding the fix.

@dimpase
Copy link
Member Author

dimpase commented Jan 7, 2026

@vbraun - this unbreaks ./bootstrap + ./configure on macOS.

@user202729
Copy link
Contributor

user202729 commented Jan 7, 2026

No idea how this is supposed to work. Surely there should be a space between -i and ''. https://stackoverflow.com/questions/7573368/in-place-edits-with-sed-on-os-x#7573438 (if you test this and it doesn't create a config.status.-E or config.status-E, it's probably fine. If it does create such a file, edit needed.)

to be safe, escape the . (it matches any character in a regex).

fixes a bug in sagemath#41333
tested on macOS and Linux now
@dimpase
Copy link
Member Author

dimpase commented Jan 7, 2026

indeed, missing space after -i caused creation of config.status-E (and probably a non0 exit code). But it did the job...

I let the pattern matching as is, as we are here at the mercy of autotools, anyway. Maybe we can just drop .'s in the pattern

@jhpalmieri
Copy link
Member

By the way, the testbots are not detecting untracked files in the repo, because with the previous version, which created config.status-E, the testbots were showing green.

@dimpase
Copy link
Member Author

dimpase commented Jan 7, 2026

I gather it's not shown due to a .gitignore rule

@jhpalmieri
Copy link
Member

I gather it's not shown due to a .gitignore rule

If I create a file config.status-E. then I see

% git status
On branch patch-18
Untracked files:
  (use "git add <file>..." to include in what will be committed)
	config.status-E

nothing added to commit but untracked files present (use "git add" to track)
% make test-git-no-uncommitted-changes 
./tools/test-git-no-uncommitted-changes
Error: the git repo has uncommitted changes:
?? config.status-E

make: *** [test-git-no-uncommitted-changes] Error 1
% echo $?
2

so it seems like it should be caught somewhere other than manually.

@jhpalmieri
Copy link
Member

Anyway, this version works for me on OS X. Can someone confirm that it is okay on a system with GNU sed?

@dimpase
Copy link
Member Author

dimpase commented Jan 8, 2026

I certainly checked Linux, and macOS

@dimpase
Copy link
Member Author

dimpase commented Jan 8, 2026

I certainly checked Linux, and macOS

sorry, I forgot running ./boostrap...

So, now, I have resorted to calling sed on macOS using the sed -i '' -E, and sed -i on the other systems. Duh...

@jhpalmieri
Copy link
Member

I don't think that the -E argument to sed is necessary on OS X. Does

sed -i '' "s|'./real_configure'|'./configure'|g" config.status

work on all supported platforms?

@dimpase
Copy link
Member Author

dimpase commented Jan 8, 2026

I don't think that the -E argument to sed is necessary on OS X. Does

sed -i '' "s|'./real_configure'|'./configure'|g" config.status

work on all supported platforms?

no, it does not work with GNU seds. One gets

sed: can't read s|'./real_configure'|'./configure'|g: No such file or directory

@jhpalmieri
Copy link
Member

We can merge this as is, although I find it annoying to have different shell code for different platforms. I think that this should be portable (although not tested), but maybe this is annoying in other ways:

sed -e "s|'./real_configure'|'./configure'|g" < config.status > config.status.TMP && mv config.status.TMP config.status

@dimpase
Copy link
Member Author

dimpase commented Jan 8, 2026

@jhpalmieri we already spent way too much time on this. 😃

@dimpase
Copy link
Member Author

dimpase commented Jan 9, 2026

@jhpalmieri - well, we can do

--- a/configure_wrapper
+++ b/configure_wrapper
@@ -14,8 +14,5 @@ cp conftest.py bak_conftest.py
 
 # Fix config.status to call ./configure instead of ./real_configure
 # so that --recheck also protects conftest.py
-if [[ "$OSTYPE" == "darwin"* ]]; then
-   sed -i '' "s|'./real_configure'|'./configure'|g" config.status
-else
-   sed -i "s|'./real_configure'|'./configure'|g" config.status
-fi
+sed -i.old "s|'./real_configure'|'./configure'|g" config.status
+rm -f config.status.old

This is portable, according to my tests. :-)


sorry, I want the 1st place in this bikesheddingfest :-)

Sign up for free to 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.

4 participants