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.

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

Fix addconfig.ac issues #186

Merged
merged 2 commits into from
Dec 14, 2016
Merged

Fix addconfig.ac issues #186

merged 2 commits into from
Dec 14, 2016

Conversation

sethfowler
Copy link
Contributor

This PR addresses a couple of small issues I noticed with the addconfig.ac workflow. Changes to extensions' aclocal.m4 files weren't being picked up even if you reran bootstrap.sh, and the addconfig.ac files were being silently ignored for users (such as myself, unfortunately) who are using non-bash-compatible shells.

@sethfowler sethfowler self-assigned this Dec 13, 2016
@@ -20,6 +20,7 @@ set -e # exit on error
./find-makefiles.sh # creates otherMakefiles.am, included in Makefile.am
mkdir -p extensions # place where additional back-ends are expected
echo "Running autoconf/configure tools"
rm aclocal.m4 # Needed to ensure we see updates to extension addconfig.ac files.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This should've been rm -f. I'll update the PR ASAP.

@@ -82,7 +82,7 @@ AX_PYTHON_MODULE([re], [fatal], [python])
dnl The following lines are just including all "addconfig.ac" files
dnl found in subdirectories

m4_foreach_w([ac_file],m4_esyscmd([find -L . -name addconfig.ac 2>/dev/null]),
m4_foreach_w([ac_file],m4_esyscmd([bash -c 'find -L . -name addconfig.ac 2>/dev/null']),
Copy link
Contributor

@ChrisDodd ChrisDodd Dec 13, 2016

Choose a reason for hiding this comment

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

Will this work on a system that does have a POSIX shell, but doesn't have bash by default (eg, OpenBSD)? I wouldn't think bash would be specifically required here, as it is just running POSIX standard find with nothing particularly tricky.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It won't, no. I could change it to sh, but I'm not 100% sure that sh accepts -c. The sh on my system is actually bash. =)

Copy link
Contributor

@ChrisDodd ChrisDodd Dec 14, 2016

Choose a reason for hiding this comment

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

POSIX sh does accept -c (http://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html#tag_18_01) so I think that's a better choice than bash. I would expect esyscmd to always use sh rather than the user's shell (you are using csh or some such?), as POSIX requires system(3) to use sh directly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks; I'll switch it over to use sh. I'm actually using fish, which unfortunately esyscmd seems to insist on using by default.

@sethfowler
Copy link
Contributor Author

OK, I made those changes. I'll go ahead and merge.

@sethfowler sethfowler merged commit 4ce9871 into p4lang:master Dec 14, 2016
@sethfowler sethfowler deleted the seth/fix-addconfig-ac-issues branch December 14, 2016 00:13
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.

2 participants