Bad file permissions and EOL encodings

The first find removes teh shebang from non-executable python files. Packaging as well as common sense requiers that a file is either executable and has a shebang, or that it's not executable and don't have it.

Hello Alec,

As you said on lp:994216, you have used on Fedora OS.
So this issue is generated on Fedora?

 I was trying to install OpenERP but did't get the success due to some packages problem.
So If you can provide the whole packages list or step, then would you please provide it.

Thanks in advance!

*I was trying to install OpenERP on Fedora but did't get the success due to some packages problem.

So If you can provide the whole packages list or step for Fedora , then would you please provide it.

Sorry for inconvenience!

On 05/04/2012 02:16 PM, Amit Parik (OpenERP) wrote:
> Hello Alec,
>
> As you said on lp:994216, you have used on Fedora OS.
> So this issue is generated on Fedora?
>
> I was trying to install OpenERP but did't get the success due to some packages problem.
> So If you can provide the whole packages list or step, then would you please provide it.
>
> Thanks in advance!
>

Hi,

The package list is one thing. Other thing is that due to Fedora
policies OE sources need to be patched, mostly because of bundling
issues. Long story short: My problems are more political than technical..

You can follow the server packaging attempt at
https://bugzilla.redhat.com/show_bug.cgi?id=817271 For the moment, I'm
busy packaging dependencies. You can find them all from that URL. The
client is at https://bugzilla.redhat.com/show_bug.cgi?id=818805. You
can see the dependencies in the spec files.

So, in the end there might be working RPM packages. But I really doubt
it will be possible to install it 'as-is'. Might be worong, for sure.

cheers,

--alec

Complete list of packaging errors. Almost all of them are related to the commands above. The exact snippet in my specfile to rectify the situation below. This is astonishly long list of commands in such a context ;)

# https://bugs.launchpad.net/bugs/993414
find . -name \*.py -a -perm 644 | \
    xargs sed -i -e '\;/usr/bin/env;d' -e '\;/usr/bin/python;d'
find . -name \*.html -o -name \*yml -o -name \*.js -o -name \*.po \
    -o -name \*.css -o -iname readme* -o -name \*.csv \
    -o -name account_asset_change_duration.py \
    -o -name base_quality_interrogation.py |
        xargs chmod 644
chmod 644 $( find openerp/addons/account_asset -name \*.py )
chmod 644 openerp/addons/l10n_ch/test/test*.v11

find . -name \*.html | xargs sed -i 's/\r//'
sed -i 's/\r//' openerp/addons/l10n_ch/test/test*.v11
sed -i 's/\r//' openerp/addons/account_asset/security/ir.model.access.csv

find . -name .hg_* | xargs rm -f
rm -f openerp/addons/.bzrignore

Hello Alec,

You are absolutely right, there are many files has a bad permission on addons and server.

I have checked this with find . -type f -perm -001 -ls command and found such kinds of file.
i.e 14687313 4 -rwxrwxrwx 1 amp amp 1511 Apr 6 10:35 ./purchase_double_validation/test/purchase_double_validation_test.yml
14690566 4 -rwxr-xr-x 1 amp amp 744 May 17 15:52 ./l10n_fr/l10n_fr_view.xml
14690576 4 -rwxr-xr-x 1 amp amp 3014 May 17 15:52 ./l10n_fr_hr_payroll/report/fiche_paye.py
14690572 60 -rwxr-xr-x 1 amp amp 57762 May 17 15:52 ./l10n_fr_hr_payroll/l10n_fr_hr_payroll_data.xml
14690573 4 -rwxr-xr-x 1 amp amp 2585 May 17 15:52 ./l10n_fr_hr_payroll/l10n_fr_hr_payroll_view.xml
14690569 4 -rwxr-xr-x 1 amp amp 1086 May 17 15:52 ./l10n_fr_hr_payroll/__init__.py
14690571 4 -rwxr-xr-x 1 amp amp 2084 May 17 15:52 ./l10n_fr_hr_payroll/l10n_fr_hr_payroll.py
14690570 4 -rwxr-xr-x 1 amp amp 2047 May 17 15:52 ./l10n_fr_hr_payroll/__openerp__.py
14685161 16 -rwxrwxrwx 1 amp amp 14451 Mar 20 13:57 ./email_template/html2text.py
14814897 4 -rwxrwxrwx 1 amp amp 57 Mar 20 13:57 ./plugin_thunderbird/static/thunderbird_plugin/install.sh
14685749 4 -rwxrwxrwx 1 amp amp 1553 Mar 20 13:57 ./document/test_cindex.py
14685319 4 -rwxrwxrwx 1 amp amp 2113 Mar 20 13:57 ./document/odt2txt.py
14814335 116 -rwxrwxrwx 1 amp amp 116090 Mar 20 13:57 ./document_ftp/ftpserver/ftpserver.py
14815791 4 -rwxrwxrwx 1 amp amp 65 Mar 20 13:57 ./base_report_designer/plugin/openerp_report_designer/bin/OOo_run.sh
14814972 4 -rwxrwxrwx 1 amp amp 1319 Mar 20 13:57 ./account_asset/security/ir.model.access.csv
14811883 4 -rwxrwxrwx 1 amp amp 1122 Mar 20 13:57 ./account_asset/wizard/__init__.py
14817509 8 -rwxrwxrwx 1 amp amp 6021 Mar 20 13:57 ./account_asset/wizard/account_asset_change_duration.py
14817507 4 -rwxrwxrwx 1 amp amp 2526 Mar 20 13:57 ./account_asset/wizard/wizard_asset_compute.py
18226614 32 -rwxrwxrwx 1 amp amp 29417 May 12 13:48 ./account_asset/i18n/de.po
18226627 28 -rwxrwxrwx 1 amp amp 25414 May 12 13:48 ./account_asset/i18n/pl.po
14814304 16 -rwxrwxrwx 1 amp amp 12686 Mar 20 13:57 ./account_asset/i18n/fr_BE.po
18226628 32 -rwxrwxrwx 1 amp amp 30010 May 12 13:48 ./account_asset/i18n/pt.po
14690985 32 -rwxrwxrwx 1 amp amp 28901 May 25 10:05 ./account_asset/i18n/fr.po
18226611 28 -rwxrwxrwx 1 amp amp 25388 May 12 13:48 ./account_asset/i18n/ca.po
18226615 32 -rwxrwxrwx 1 amp amp 30102 May 12 13:48 ./account_asset/i18n/es.po
18226617 32 -rwxrwxrwx 1 amp amp 30123 May 12 13:48 ./account_asset/i18n/es_CR.po
18226633 24 -rwxrwxrwx 1 amp amp 21098 May 12 13:48 ./account_asset/i18n/sv.po
14685750 28 -rwxrwxrwx 1 amp amp 27941 Mar 20 13:57 ./document_webdav/test_davclient.py
14943...

Maybe I should add that it's not just permissions. It's also about non-consistent EOL encoding; most files uses *ux EOL (i. e., a single '\n') but some random files have DOS/windows '\r\n'. The sed commands near the bottom handles this.

And finally, there are version control files leftovers, that's the 'rm,' commands at bottom . I presume you don't intend to distribute version control files in the released version?!)

The situation is is just slightly improved in 7.0 (the inconsistent line encodings are gone). Current code to rectify situation:

# https://bugs.launchpad.net/bugs/993414
find . -name \*.py -a -perm 644 -a -type f | \
    xargs sed -i -e '\;/usr/bin/env;d' -e '\;/usr/bin/python;d'
sed -i -e '\;/usr/bin/env;d' \
    openerp/addons/l10n_fr_hr_payroll/report/fiche_paye.py
find . \( -name \*.html -o -name \*yml -o -name \*.js -o -name \*.po \
            -o -name \*.css -o -iname readme* -o -name \*.csv \
            -o -name \*.xml -o -name \*.svg \
            -o -name account_asset_change_duration.py \
            -o -name input_complete \
            -o -name base_quality_interrogation.py \
        \) -a -type f |
    xargs chmod 644
chmod 644 $( find openerp/addons/account_asset -type f -name \*.py )
chmod 644 $( find openerp/addons/l10n_fr_hr_payroll -type f -name \*.py )
chmod 644 $( find openerp/addons//l10n_ro -type f -name \*.py )

This handles:
- .py library files (not scripts) with a #/usr/bin/env or /usr/bin/python shebang by removing the shebang. A shebang on a library
file makes no sense.
- All sorts of files with -x permission which shouldn't have that (.svg, .po, .js, .csv etc.)

This is still a mess. I understand this is a low priority bug, but would nevertheless appreciate if it was fixed. The lines above is the closest to a patch I can come, though.

We'll have to handle Python files carefully if we don't want to mess up things even more ;-)
I see:
- quite a few library files that have a shebang but do not need it
- quite a few scripts that need to be marked executable and whose shebang should not be removed (e.g. openerp_mailgate.py)
- at least one script that is executable but does not have the shebang! (base_quality_interrogation.py)

I don't mind scripting the chmod on non-python files, but both executable python files and python files with a shebang at least need to be reviewed one by one afterwards. To speed things up, it would help if someone could make a draft merge proposal with these changes, making the result easier to review.

Seems that we agree that this is a mess that should be resolved :)

Can you handle a git diff? For me, this is the natural way to get a reviewable diff describing also permsissions. I could try to create a patch series if it's OK.

Patch series corresponding to the scripted commands, with some modifications after comments

These are git patches, use e. g., 'git apply' to use them. (plain diff/patch does not handle permissions).

While handling these overall sanity issues, perhaps you could consider applying the patch in bug 993408 as well? (my last one of this kind, promise!)

Hello,

I am reviewing your patches (thank you for that). Some of the permissions have already been fixed and I will apply the other soon. However before doing that, I had some questions :
- why would you want to add a shebang in addons/l10n_ro/res_partner.py ?
- Some other files something2txt.py should be marked as executable and need shebang
- not sure I understand your last patch with EOL. I don't seem to have this problem with the current code

Regards

res_partner.py: I might certainly have got this wrong, for sure. That said, my conclusion came from the 755 permissions + the last line which creates a transient class instance. If this is jst a library , which it indeed seems to be at a second thought, it should of course be 644, no shebang and perhaps also without the last line executed at import time.

something2txt et. a.: My packaging tools just don't catch these, they just spot inconsistent combinations of permissions and shebang. I'm sure you are in a position to judge this properly ;)

EOL-patch: this is what I see (on -20130424-232422)

$ cd openerp/addons/web_calendar/static/lib/dhtmlxScheduler

[al@muminmamman dhtmlxScheduler]$ file license.txt

license.txt: Pascal source, ASCII text, with CRLF line terminators

CRLF is not kosher here, should be just LF (as the other text files).

Ok I see the problem with CRLF. Thank you for your patches. These changes have been applied and merged into v7 (server, addons and web).
If you notice other changes, let us know :)

Thanks for fixing this mess. Would it be too much to remind about comment #17 while handling this boring, sanity stuff? It should be easy, really.

BTW: If not already done, a push hook taking care about trailing whitespace, bad EOL, bad permissions and bad combinations permissions/shebang is certainly an idea, I guess.

You mean the bug related to the fsf addresses ? I fixed it in the same commit :)
I agree it would be good to avoid these kind of errors in the future (like when we will forward port this fix in trunk)...

The fix has landed in
- 7.0 server at revision 4974 (rev-id: <email address hidden>)
- 7.0 addons at revision 9123 (<email address hidden>)
- 7.0 web at revision 3921 (<email address hidden>)

As one might expect, something is missed in such a huge patch:

 openerp7.noarch: E: non-executable-script /usr/lib/python2.7/site-packages/openerp/tests/test_mail.py 0644L /usr/bin/en

To clarify, this is not a problem for me to fix downstream. But since we set off to fix this once and for all, we should perhaps fix this as well.

Fixed test_mail.py in server 7.0 branch at revision 4976 (rev-id: <email address hidden>), sorry we missed that one.

I performed an extra sanity check on all 7.0 branches and 2 new files (merged since the bug was fixed) had to be fixed again: l10n_vn/__init__.py and l10n_vn/__openerp__.py. Did this in addons 7.0 branch at revision 9137 (rev-id: <email address hidden>).

I would really love to be able to set commit-hooks that would catch this, unfortunately Launchpad does not support that, even though Bazaar technically does to some level. And we can't move to another platform easily because we need very specific features that make popular alternatives unsuitable for us (in terms of Bug Tracker organization and integrated translation system, among others)

Thanks for your patience!

Thanks for taking care of this!

To iterate, to have to fix file permissions for some files is quite normal when packaging and really not a problem. What made me raise the issue was the sheer amount of fixes needed, making it hard to maintain the spec.

Of course, moving to from lp is not an option in this context. But since you cannot add a commit hook, perhaps some kind of automated, nightly sanity report would make it easier to track this? Eventually, you might even get rid of the trailing whitespace :)

Someone should file a RFE bug against lp asking them to implement hooks....

Just FYI, fixes needed one month later (not to bad IMHO):

chmod 644 openerp/addons/l10n_vn/*.xml
sed -i -e '1i#!/usr/bin/env python' \
    openerp/addons/base_report_designer/openerp_sxw2rml/openerp_sxw2rml.py