[!]: License field in the package spec file matches the actual license.
     Note: Checking patched sources after %prep for licenses. Licenses found:
     "BSD (3 clause)", "Unknown or generated". 9 files have unknown license.

freecode says "BSD Revised", spec file says MIT, LICENSE file contains license texts, that looks to me BSD-like.  freecode page is being maintained by upstream, so I'd say it's "BSD".


And not really a bug, just complains from fedora-review:

[!]: Spec file according to URL is the same as in SRPM.
     Note: Spec file as given by url is not the same as in SRPM

 %description
-Osh is an enhanced, backward-compatible port of the Sixth Edition Thompson 
-shell. Sh6 is an unenhanced port of the shell, and glob6 is a port of 
+Osh(1) is an enhanced, backward-compatible port of the Sixth Edition Thompson 
+shell. Sh6(1) is an unenhanced port of the shell, and glob6(1) is a port of 
 its global command. Together, sh6 and glob6 provide a user interface which is

Everything else is OK.

(In reply to Alexey I. Froloff from comment #1)
> [!]: License field in the package spec file matches the actual license.
>      Note: Checking patched sources after %prep for licenses. Licenses found:
>      "BSD (3 clause)", "Unknown or generated". 9 files have unknown license.
> 
> freecode says "BSD Revised", spec file says MIT, LICENSE file contains
> license texts, that looks to me BSD-like.  freecode page is being maintained
> by upstream, so I'd say it's "BSD".

Corrected.

> And not really a bug, just complains from fedora-review:
> 
> [!]: Spec file according to URL is the same as in SRPM.
>      Note: Spec file as given by url is not the same as in SRPM
> 
>  %description
> -Osh is an enhanced, backward-compatible port of the Sixth Edition Thompson 
> -shell. Sh6 is an unenhanced port of the shell, and glob6 is a port of 
> +Osh(1) is an enhanced, backward-compatible port of the Sixth Edition
> Thompson 
> +shell. Sh6(1) is an unenhanced port of the shell, and glob6(1) is a port of 
>  its global command. Together, sh6 and glob6 provide a user interface which
> is

XD I used this as the first version, then I deleted those manpage numbers. I'd like to keep the current style.

NEW SPEC URL: http://us-la.cicku.me/osh.spec
NEW SRPM URL: http://us-la.cicku.me/osh-20140410-2.fc21.src.rpm

Oops, I'm too late.  I was supposed to take this review in exchange.  Sorry to be slow.  Anyway, I found a few more issues that need to be addressed:

1. The debuginfo package is empty, because install is invoked with the -s option.

2. Parallel make isn't working correctly.  The build log shows the same file being compiled by multiple threads; e.g., v.c was built 6 times for me.

3. The package BuildRequires ncurses-devel, but the binary package doesn't depend on any of the ncurses libraries.  I also don't see any #include directives that need ncurses present.

Finally, I am concerned about introducing /usr/bin/if.  That's not only a generic name, but a keyword in pretty much every scripting language in existence.  The fact that it is a keyword *probably* means that introducing it as a binary is okay, but ... eek!  The name /usr/bin/goto is also somewhat generic.  The NOTES file says these are not needed by osh(1), but only by sh6(1).  Can we hide them away somehow, perhaps with environment modules?

Christopher, if you want to hand me another package for a review swap, please feel free to do so.

(In reply to Jerry James from comment #4)
> Oops, I'm too late.  I was supposed to take this review in exchange.  Sorry
> to be slow.  Anyway, I found a few more issues that need to be addressed:

No problem.

> 1. The debuginfo package is empty, because install is invoked with the -s
> option.
 
I will fix that. Didn't spot that.

> 2. Parallel make isn't working correctly.  The build log shows the same file
> being compiled by multiple threads; e.g., v.c was built 6 times for me.

Really? I haven't noticed that as well on my slow computer.

> 3. The package BuildRequires ncurses-devel, but the binary package doesn't
> depend on any of the ncurses libraries.  I also don't see any #include
> directives that need ncurses present.

My mistake, mixed with other packages. ;)

> Finally, I am concerned about introducing /usr/bin/if.  That's not only a
> generic name, but a keyword in pretty much every scripting language in
> existence.  The fact that it is a keyword *probably* means that introducing
> it as a binary is okay, but ... eek!  The name /usr/bin/goto is also
> somewhat generic.  The NOTES file says these are not needed by osh(1), but
> only by sh6(1).  Can we hide them away somehow, perhaps with environment
> modules?

osh is designed to run as an alternative of other shells, but not running itself in other shells.

I've discussed this with upstream already ;) Anyway I will forward this to him and await his answer.

===============================8<================================
"In short, sh(1) can give errors for if(1) commands because "if" for sh(1) and osh(1) differ in both behavior and syntax. A correct sh script will find the sh "if" command first because it is a built-in, as a correct osh script will find the osh "if" command first for the same reason.

The sh6 "if" is external, by the way."

"Assuming the environment under which the current instance of bash is running is configured correctly (e.g., `pwd`, `umask`, $PATH, etc), I don't see a problem. Bash is gonna find its "if" unless the script or user jumps through hoops to make it fail. Take these simple examples running under an interactive bash instance:

$ which bash csh osh sh
/bin/bash
/bin/csh
/usr/local/bin/osh
/bin/sh

$ bash -c if ; echo $?
bash: -c: line 1: syntax error: unexpected end of file
1

$ csh -c if ; echo $?
if: Too few arguments.
1

$ osh -c if ; echo $?
1

$ sh -c if ; echo $?
sh: 1: Syntax error: end of file unexpected
2"

===============================>8================================

> Christopher, if you want to hand me another package for a review swap,
> please feel free to do so.

Sure, take a look at bug 1115881, it might be painful XD.

Or easier: bug 1116071

I also think that /usr/bin/if and /usr/bin/goto is OK here.

Christofer, can osh be used as login shell?  If yes, it should be added/remove to /etc/shells like other shells (zsh for example) do.

(In reply to Alexey I. Froloff from comment #6)
> I also think that /usr/bin/if and /usr/bin/goto is OK here.
IMO, they are not OK, because they can interfere with other shells and potentially disturbe them.

1. How binary can interfere with builtin?
2. What shell have goto?

(In reply to Alexey I. Froloff from comment #8)
> 1. How binary can interfere with builtin?
E.g. by explictly calling them.

IIRC, some shells have settings to manipulate search orders.

Also take into account that "if" is not amongst those shell commands which POSIX are guarantes to be built-in. I.e. your if may clash with another shell's or other arbitrary tools external "if" at any time.

> 2. What shell have goto?
None that I am aware about, but other languages may have it and may pickup tools from $PATH.


In any case both names, "if" and "goto" are way too common names to allow them to be put inside in a standard path.

I'd suggest to delegate this issue to FPC and have it discussed there.

I am not familiar with osh's implementation details, but for now, I'd suggest to install these osh-private helper programs into /usr/libexec/osh/.... and modify osh in such a way it picks up these tools there.

(In reply to Ralf Corsepius from comment #9)
> > 1. How binary can interfere with builtin?
> E.g. by explictly calling them.
How?  /usr/bin/if?  What's wrong with that?

> IIRC, some shells have settings to manipulate search orders.
Pleas name at least two.  And show example of your so-called "interferention".

> Also take into account that "if" is not amongst those shell commands which
> POSIX are guarantes to be built-in. I.e. your if may clash with another
> shell's or other arbitrary tools external "if" at any time.
According to SUSv3, "if" is a reserverd word. "Reserved words are words that have special meaning to the shell".  It is not even a builtin.

> > 2. What shell have goto?
> None that I am aware about, but other languages may have it and may pickup
> tools from $PATH.
Care to show some examples, how /usr/bin/goto "breaks everything"?  Or maybe you want to take over this review?

(In reply to Christopher Meng from comment #5)
> > 2. Parallel make isn't working correctly.  The build log shows the same file
> > being compiled by multiple threads; e.g., v.c was built 6 times for me.
> Really? I haven't noticed that as well on my slow computer.

This is because several targets call "@$(MAKE) $@bin".  This spawns several make processes and all of them starts making v.o...

This is an automatic check from review-stats script.

This review request ticket hasn't been updated for some time, but it seems
that the review is still being working out by you. If this is right, please
respond to this comment clearing the NEEDINFO flag and try to reach out the
submitter to proceed with the review.

If you're not interested in reviewing this ticket anymore, please clear the
fedora-review flag and reset the assignee, so that a new reviewer can take
this ticket.

Without any reply, this request will shortly be resetted.

This is an automatic action taken by review-stats script.

The ticket reviewer failed to clear the NEEDINFO flag in a month.
As per https://fedoraproject.org/wiki/Policy_for_stalled_package_reviews
we reset the status and the assignee of this ticket.

This is an automatic action taken by review-stats script.

The ticket submitter failed to clear the NEEDINFO flag in a month.
As per https://fedoraproject.org/wiki/Policy_for_stalled_package_reviews
we consider this ticket as DEADREVIEW and proceed to close it.