Opened 3 years ago

Closed 3 years ago

Last modified 3 years ago

#19333 closed Bug (fixed)

Collect static copies a .py file

Reported by: camilonova Owned by: nobody
Component: contrib.admin Version: 1.4
Severity: Normal Keywords:
Cc: Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

When you run a collectstatic the file compress.py gets copied to the static directory, and that doesn't seem right.

I have created a pull request because this was generating PEP8 warnings for a project, but looking at the comments i agree this file shouldn't be copied.

https://github.com/django/django/pull/474

Maybe some can tweak the collectstatic command so it ignores .py files.

Change History (16)

comment:1 Changed 3 years ago by aaugustin

  • Component changed from contrib.staticfiles to contrib.admin
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Accepted
  • Type changed from Cleanup/optimization to Bug

Yes, this file isn't designed to be served, it should be moved outside of the admin's static directory.

comment:2 Changed 3 years ago by jezdez

Or we could add .py to the list of ignore file extensions.

comment:3 Changed 3 years ago by camilonova

Ignoring the .py file extensions sounds a better idea overall

comment:4 Changed 3 years ago by aaugustin

My personal website offers some Python scripts for download. I'm currently storing them as static files. If .py files were ignored by collectstatic they wouldn't be served anymore.

I may be abusing the static files framework :) but it's possible that other people are doing the same thing. For this reason, "ignore .py files" wasn't my first choice, but I would understand it.

comment:5 Changed 3 years ago by russellm

Can anyone explain why the compress.py file is actually in the static directory to start with? If the static directory is the directory of stuff you want to share, It seems to me that we could avoid this whole problem by putting compress.py somewhere else (like a utilities directory -- which is what compress.py is anyway)

comment:6 Changed 3 years ago by ramiro

  • Has patch set

comment:7 Changed 3 years ago by camilonova

Ramiro, looks good and clean. Thanks

comment:8 follow-up: Changed 3 years ago by aaugustin

After your changes the script works only if you pass file names in argument or if you run it from django/contrib/admin/static/admin/js.

The here variable should be called admin_js_dir, because that's what it really is :)

I suggest renaming that variable and pointing it to os.path.join(os.path.dirname(os.path.dirname(__file__)), 'static', 'admin', 'js') (to be tested, I just wrote it in this comment as an example).

Otherwise, that's the correct fix IMO.

Last edited 3 years ago by aaugustin (previous) (diff)

comment:9 in reply to: ↑ 8 Changed 3 years ago by ramiro

Replying to aaugustin:

After your changes the script works only if you pass file names in argument or if you run it from django/contrib/admin/static/admin/js.

According to my understanding of the code it's also the current behavior. No changes here.

The here variable should be called admin_js_dir, because that's what it really is :)

If I'm right above, the meaning of this var hasn't changed either. It is the CWD from which the script is being executed (not the dir where the script is located).

I suggest renaming that variable and pointing it to os.path.join(os.path.dirname(os.path.dirname(__file__)), 'static', 'admin', 'js') (to be tested, I just wrote it in this comment as an example).

Otherwise, that's the correct fix IMO.

The only change this patch introduces is that to ejecute compress.py one needs to specify its full path (e.g. python ../../../bin/compress.py). Remember the intended potential uses of this script are core devs and people contributing with the admin app by submitting JS patches.

Last edited 3 years ago by ramiro (previous) (diff)

comment:10 Changed 3 years ago by julien

  • Triage Stage changed from Accepted to Ready for checkin

comment:11 Changed 3 years ago by julien

Never mind, I got a little confused. The patch already modifies the doc...

On second thought, I agree with Aymeric in comment:8 to not use getcwd(), so that the command can be run from anywhere.

comment:12 Changed 3 years ago by julien

  • Triage Stage changed from Ready for checkin to Accepted

comment:13 Changed 3 years ago by ramiro

  • Triage Stage changed from Accepted to Ready for checkin

comment:14 Changed 3 years ago by Julien Phalip <jphalip@…>

  • Resolution set to fixed
  • Status changed from new to closed

In c9c40bc6bc64e67365338751e4967d86d0882abf:

Fixed #19333 -- Moved compress.py outside of the admin static folder. Thanks to camilonova, Russell Keith-Magee, Aymeric Augustin and Ramiro Morales for the feedback.

comment:15 Changed 3 years ago by Julien Phalip <jphalip@…>

In be5369fd24adfc788e59eaf0363d4d137ded6ef8:

[1.5.x] Fixed #19333 -- Moved compress.py outside of the admin static folder. Thanks to camilonova, Russell Keith-Magee, Aymeric Augustin and Ramiro Morales for the feedback.
Backport of c9c40bc6bc64e6

comment:16 Changed 3 years ago by Julien Phalip <jphalip@…>

In be5369fd24adfc788e59eaf0363d4d137ded6ef8:

[1.5.x] Fixed #19333 -- Moved compress.py outside of the admin static folder. Thanks to camilonova, Russell Keith-Magee, Aymeric Augustin and Ramiro Morales for the feedback.
Backport of c9c40bc6bc64e6

Note: See TracTickets for help on using tickets.
Back to Top