Message ID | 1443175585-27077-1-git-send-email-Vincent.Riera@imgtec.com |
---|---|
State | Superseded |
Headers | show |
Vicente, I think the patch is great, but there are some implementation issues, see below. On Fri, 25 Sep 2015 11:06:25 +0100, Vicente Olivert Riera wrote: > -# Check that the Perl installation is complete enough to build > -# host-autoconf. > -if ! perl -e "require Data::Dumper" > /dev/null 2>&1 ; then > - echo "Your Perl installation is not complete enough, at least Data::Dumper is missing." > - echo "On Debian/Ubuntu distributions, install the 'perl' package." > +# Check that the Perl installation is complete enough for Buildroot. > +# Here is the space-separated list of the required modules. The space-separated comment doesn't make much sense IMO. > +required_perl_modules="Data::Dumper" # Needed to build host-autoconf > +required_perl_modules+=" Thread:Queue" # Used by host-automake The += operator is bash specific, and not POSIX compliant. Since this dependencies.sh script specifies #!/bin/sh, you're not guaranteed to be executed with bash. > + > +# This variable will keep the modules that are missing in your system. > +missing_perl_modules="" > + > +for pm in $required_perl_modules ; do > + if ! perl -e "require $pm" > /dev/null 2>&1 ; then > + missing_perl_modules+=" $pm" Ditto. > + fi > +done > + > +if [ -n "$missing_perl_modules" ] ; then > + echo "Your Perl installation is not complete enough; at least the following" > + echo "modules are missing:" > + echo "" Quotes not needed here, just "echo" is sufficient. > + for pm in $missing_perl_modules ; do > + echo -e "\t $pm" > + done > + echo "" Ditto. Thomas
Hi Thomas, thanks a lot for your review. See comments below, please. On 09/28/2015 09:56 PM, Thomas Petazzoni wrote: > Vicente, > > I think the patch is great, but there are some implementation issues, > see below. > > On Fri, 25 Sep 2015 11:06:25 +0100, Vicente Olivert Riera wrote: > >> -# Check that the Perl installation is complete enough to build >> -# host-autoconf. >> -if ! perl -e "require Data::Dumper" > /dev/null 2>&1 ; then >> - echo "Your Perl installation is not complete enough, at least Data::Dumper is missing." >> - echo "On Debian/Ubuntu distributions, install the 'perl' package." >> +# Check that the Perl installation is complete enough for Buildroot. >> +# Here is the space-separated list of the required modules. > > The space-separated comment doesn't make much sense IMO. Uhm..., ok. I think having two examples (Data::Dumper and Thread:Queue) is enough to understand how to add more modules to the list. I will remove the comment. >> +required_perl_modules="Data::Dumper" # Needed to build host-autoconf >> +required_perl_modules+=" Thread:Queue" # Used by host-automake > > The += operator is bash specific, and not POSIX compliant. Since this > dependencies.sh script specifies #!/bin/sh, you're not guaranteed to be > executed with bash. Totally true, I will fix that. Thanks! >> + >> +# This variable will keep the modules that are missing in your system. >> +missing_perl_modules="" >> + >> +for pm in $required_perl_modules ; do >> + if ! perl -e "require $pm" > /dev/null 2>&1 ; then >> + missing_perl_modules+=" $pm" > > Ditto. Noted. >> + fi >> +done >> + >> +if [ -n "$missing_perl_modules" ] ; then >> + echo "Your Perl installation is not complete enough; at least the following" >> + echo "modules are missing:" >> + echo "" > > Quotes not needed here, just "echo" is sufficient. Yeah, that's right. >> + for pm in $missing_perl_modules ; do >> + echo -e "\t $pm" >> + done >> + echo "" > > Ditto. Noted. I will send a new version today. Regards, Vincent. > Thomas >
diff --git a/support/dependencies/dependencies.sh b/support/dependencies/dependencies.sh index 01ad828..8ff667c 100755 --- a/support/dependencies/dependencies.sh +++ b/support/dependencies/dependencies.sh @@ -236,10 +236,27 @@ if grep -q ^BR2_HOSTARCH_NEEDS_IA32_COMPILER=y $BR2_CONFIG ; then fi fi -# Check that the Perl installation is complete enough to build -# host-autoconf. -if ! perl -e "require Data::Dumper" > /dev/null 2>&1 ; then - echo "Your Perl installation is not complete enough, at least Data::Dumper is missing." - echo "On Debian/Ubuntu distributions, install the 'perl' package." +# Check that the Perl installation is complete enough for Buildroot. +# Here is the space-separated list of the required modules. +required_perl_modules="Data::Dumper" # Needed to build host-autoconf +required_perl_modules+=" Thread:Queue" # Used by host-automake + +# This variable will keep the modules that are missing in your system. +missing_perl_modules="" + +for pm in $required_perl_modules ; do + if ! perl -e "require $pm" > /dev/null 2>&1 ; then + missing_perl_modules+=" $pm" + fi +done + +if [ -n "$missing_perl_modules" ] ; then + echo "Your Perl installation is not complete enough; at least the following" + echo "modules are missing:" + echo "" + for pm in $missing_perl_modules ; do + echo -e "\t $pm" + done + echo "" exit 1 fi
Signed-off-by: Vicente Olivert Riera <Vincent.Riera@imgtec.com> --- Changes v5 -> v6: - Remove the check for perl itself, as is already done at line #161. Changes v4 -> v5: - Use the string concatenation operator (+=) to assing the values to the required_perl_modules. That way the comments about why each module is needed can be in the same line where the module is added to the variable. Use also += for the missing_perl_modules variable. Changes v3 -> v4: - Thread::Queue module is actually needed because host-automake uses it. Comment modified accordingly. (Highlighted by Thomas Petazzoni) Changes v2 -> v3: - Add comments about why each module is needed. (Suggested by Baruch Siach) Changes v1 -> v2: - Check first for perl itself, and then the modules. Also keep the comment which says that perl is needed to build host-autoconf. (Suggested by Baruch Siach) support/dependencies/dependencies.sh | 27 ++++++++++++++++++++++----- 1 files changed, 22 insertions(+), 5 deletions(-)