[Bug 1202604] Review Request: python-sscg - Self-signed Certificate Generator

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



https://bugzilla.redhat.com/show_bug.cgi?id=1202604



--- Comment #8 from Stephen Gallagher <sgallagh@xxxxxxxxxx> ---
(In reply to Zbigniew Jędrzejewski-Szmek from comment #5)
> Running 2to3 over the sources does not actually do anything, except remove
> some imports from __future__. So the 2to3 step can be skipped. And this
> allows nice simplifications to be made in packaging, because if 2to3 is not
> run, there's no build step, and the install step for Python 2 and 3 can be
> run from the source directory.
> 
> I also think providing two binaries (for Python 2 and 3) is just confusing.
> Installing one which runs under whichever version of python is default
> should be enough.

Well, I was trying to address a potential containerized case where either
python release might be available. If I was only going to include the script
for the default python, it doesn't make sense to build both (this module isn't
really intended to be imported by other scripts, though it could be). So if
you're building a Dockerfile and want the image to generate a certificate on
startup, you probably want to be able to use whichever version of the script
matches the version of python already in the environment.

> 
> So the whole procedure could look like:
> %prep
> %setup ...
> rm -rf src/*.egg-info
> 
> %build
> %{__python2} setup.py build
> 
> %install
> %{__python2} setup.py install --skip-build --root=%{buildroot}
> %{__python3} setup.py install --skip-build --root=%{buildroot}
> # or in reverse order
> 
> This provides proper #! lines.
> 

Yeah, actually the sed thing is a leftover, since I don't have any #! lines in
my actual code; just in the generated scripts.


> --
> 
> In the python code, a bare try: ... except: (without any exception name
> after "except") is used. In some places the exception is re-raised, but in
> others not. This is a problem because the script might be ended with ^C (or
> a signal) and silently "swallow" the exception. The script should always
> exit after being interrupted. It also should not print an error message like
> "Failed to write..." when killed.
> 


Ah, where did I do that? It wasn't intentional.

> It seems that gettext is called on formatted arguments:
>    _("Could not write to {0}. Error: {1}".format(...))
> but it should be before:
>    _("Could not write to {0}. Error: {1}").format(...)
> 

Typo, thanks for catching it.

> Why open the temp file in text mode, then decode and re-encode the
> certificate. Maybe it would be simpler to open it in binary mode and skip
> the decode step?
> 

I don't do I? I thought the default was binary mode, which is why I was
decoding the UTF-8 to the binary representation. Or have I confused myself? (I
don't like dealing with encodings...)


> It seems that this will be usable a library. I think that calling sys.exit()
> should not be done, except in main. Errors should not be printed either.
> Simply raising an exception with an appropriately formatted message and
> letting the caller handle it would nicely simplify things.

It's not supposed to be used directly as a library, only as a script.

-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
_______________________________________________
package-review mailing list
package-review@xxxxxxxxxxxxxxxxxxxxxxx
https://admin.fedoraproject.org/mailman/listinfo/package-review





[Index of Archives]     [Fedora Legacy]     [Fedora Desktop]     [Fedora SELinux]     [Yosemite News]     [KDE Users]     [Fedora Tools]