[Bug 1014336] Review Request: python-halite - Web GUI for SaltStack

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

 



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



--- Comment #6 from Erik Johnson <erik@xxxxxxxxxxxxx> ---
(In reply to Mario Blättermann from comment #5)
> First, I'm not really satisfied with the behavior of your download links.
> Reviewers should be able to simply right-click on a link to save its target.
> Dropbox allows this actually, but it seems you've copied the file link from
> your browser's address bar. This doesn't point to the file itself, rather to
> a website where I have to click on a button to download it. To avoid this,
> open your Dropbox in the browser, select the desired file and click on "Copy
> public link" (sorry, could be some different, I'm using Dropbox in German).
> Even the available file manager plugins for Nautilus, Dolphin etc. should
> have such an option.
> 

Well, I do not use a file manager, and Dropbox has long since gotten rid of the
"get public link" option. The only option now is a "share link" option, which
provides the URLs I specified.

However, I did some digging and the the public direct links are available if
you right-click the "Download" button on the links I originally posted, and
copy the URL. Given that knowledge, here are the public download links:

Spec URL:
https://dl.dropboxusercontent.com/s/dyrl1jkyiwhhzxj/python-halite.spec
SRPM URL:
https://dl.dropboxusercontent.com/s/4shqedab21j894f/python-halite-0.1.01-1.el6.src.rpm


> 
> Some of the issues need to be fixed or investigated:
> 
> I don't know why $RPM_SOURCE_DIR is not allowed, but I assume there are good
> reasons for. It is unusual at least, never seen this before in a spec file.
> Just use %RPM_BUILD_ROOT instead.
> 

This has been replaced.

> There are some scripts which have a shebang, but are not executable. Scripts
> in %{python_sitelib} don't need a shebang. Please remove them.
> 

Done. And I've notified upstream about this as well, thanks.

> Don't mix spaces and tabs. I recommend spaces, because this way the spec
> file looks the same in any text editor, independent from the configured tab
> width.
> 

Fixed.

> The download location for the license file is not available, please
> investigate.
> 

I used ${version} instead of %{version}. In addition, I should have used the
"raw.github.com" link to get the raw text, so I fixed that as well.

> 
> %if 0%{?fedora} >= 8
> BuildRequires:  python-setuptools-devel
> %else
> BuildRequires:  python-setuptools
> %endif
> 
> First, we don't need any definitions which refer to Fedora 7 and earlier.
> Well, you could apply this condition to EPEL 5 which is based on FC6.
> Besides that, python-setuptools-devel is virtually provided by
> python-setuptools. As far as I can see, this definition can be omitted.
> 
> 

Removed, thanks.

> Consider to use %global instead of %define. See
> https://fedoraproject.org/wiki/Packaging:Guidelines?rd=Packaging/Guidelines#.
> 25global_preferred_over_.25define for more information.
> 

Fixed.

> 
> Last but not least, please get rid of the spec parts which are only needed
> for EPEl 5 once your package will be imported into EPEL >= 6 and Fedora:
> 
> * BuildRoot definition
> * The header which defines python_sitelib
> * Initial cleaning of buildroot in %install
> * The %clean section
> * %defattr line in %files

I'm sorry, but I had trouble understanding what you meant here. Do you mean
that these parts are not needed anymore? Or that they should be enclosed within
%if blocks like so:

%if ! (0%{?rhel} >= 6 || 0%{?fedora} > 12)
BuildRoot: %{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n)
%endif

-- 
You are receiving this mail because:
You are on the CC list for the bug.
Unsubscribe from this bug https://bugzilla.redhat.com/token.cgi?t=KtjlpkydRx&a=cc_unsubscribe
_______________________________________________
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]