Opened 8 years ago

Closed 4 years ago

#14728 closed enhancement (wontfix)

cython dependency tracking

Reported by: felixs Owned by: felixs
Priority: major Milestone: sage-duplicate/invalid/wontfix
Component: cython Keywords: dependencies, cython, makefile
Cc: robertwb, jondo Merged in:
Authors: Reviewers: Jeroen Demeyer
Report Upstream: Reported upstream. Developers acknowledge bug. Work issues:
Branch: u/felixs/cython_deps (Commits, GitHub, GitLab) Commit: bd11c246d0df90dac0fa2f362c6d4e961cece5ad
Dependencies: Stopgaps:

Status badges

Description

cython does not write out make rules for dependencies, like other code-processors to. this complicates dependency tracking a lot.

this is a known bug, http://trac.cython.org/cython_trac/ticket/655

my patch (minimal changes required in frontend) might need some scrubbing, but it works well for me.

Change History (24)

comment:1 Changed 8 years ago by leif

  • Authors set to Felix Salfelder

comment:2 follow-up: Changed 8 years ago by leif

Minor comments:

I'd go / stay with single-letter short options (i.e., -M <makefile> or e.g. --makefile <makefile>).

I'd use tabs (which is also the portable way) instead of hardcoded " " (four spaces).

comment:3 in reply to: ↑ 2 ; follow-ups: Changed 8 years ago by felixs

Replying to leif:

Minor comments:

I'd go / stay with single-letter short options (i.e., -M <makefile> or e.g. --makefile <makefile>).

i dont like -MF either (does it violate POSIX?), but thats what gcc (and others) do. -M is something else -- related but different -- and will cause confusion. there are even other -M[A-Z] options that might be useful. i would just stick to long options, if they didn't waste so much space...

I'd use tabs (which is also the portable way) instead of hardcoded " " (four spaces).

gcc writes exactly one space character. i will do it like that, as it minimizes wrapping.

comment:4 in reply to: ↑ 3 ; follow-up: Changed 8 years ago by leif

Replying to felixs:

Replying to leif:

I'd go / stay with single-letter short options (i.e., -M <makefile> or e.g. --makefile <makefile>).

i dont like -MF either (does it violate POSIX?)

MF is at least not politically correct.

but thats what gcc (and others) do.

Well, that's cython, not GCC, and Cython (so far) uses either single-letter short options or long options, so one dayTM -MF might get interpreted as -M -F, with a different meaning.

-M is something else -- related but different

It just outputs the dependencies to stdout.


I'd use tabs (which is also the portable way) instead of hardcoded " " (four spaces).

gcc writes exactly one space character. i will do it like that, as it minimizes wrapping.

Oh, you're right; we're writing just (escaped single-line) dependencies as opposed to receipts.

comment:5 in reply to: ↑ 4 Changed 8 years ago by felixs

MF is at least not politically correct.

Exactly, that's why i dont like it. on second thought it cannot cause problems, if -MF (gcc) translates to -F (cython, unused) and -M (gcc) will be implemented as -M (on cython side, currently unused).

but thats what gcc (and others) do.

-MF might get interpreted as -M -F, with a different meaning.

clearly, -MF will be interpreted as -M -F but not with a different meaning. For gcc "-M -MF <file>" is also equal to "-MF <file>". thats the same with the other -M[A-Z] options as far as i can see.

I would favor wasting the -F short option to implement gcc-style, rather than choosing new option names for well established functionality. but my patch needs to reflect that, and allow "-F" as well.

unfortunately i'm not the one who will decide, so i will have to contact upstream about it.

comment:6 in reply to: ↑ 3 ; follow-up: Changed 8 years ago by leif

Replying to felixs:

Replying to leif:

I'd go / stay with single-letter short options (i.e., -M <makefile> or e.g. --makefile <makefile>).

i dont like -MF either (does it violate POSIX?)

Single Unix Specification Utility Syntax Guidelines

"It is recommended that all future utilities and applications use these guidelines to enhance user portability. The fact that some historical utilities could not be changed (to avoid breaking existing applications) should not deter this future goal."

:-)

comment:7 in reply to: ↑ 6 ; follow-up: Changed 8 years ago by felixs

Replying to leif:

Single Unix Specification Utility Syntax Guidelines

Thanks. I was looking for something like it.

You are not implying that the quote is opposed to my previous post, are you?

comment:8 in reply to: ↑ 7 ; follow-up: Changed 8 years ago by leif

Replying to felixs:

Replying to leif:

Single Unix Specification Utility Syntax Guidelines

Thanks. I was looking for something like it.

See also man 3 getopt (and for the GNU version, especially POSIXLY_CORRECT ;-) )


You are not implying that the quote is opposed to my previous post, are you?

I leave that to your interpretation.

Note that GNU cpp / gcc / g++ do not accept -MF <outputfile> alone; you have to also give -M or -MM in addition.

(And you'd probably also have to support -FM ..., and ignore -F.)

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

Note that GNU cpp / gcc / g++ do not accept -MF <outputfile> alone; you have to also give -M or -MM in addition.

Indeed. Particularly I am wrong in post number 5 about the gcc options. -MM is be not very much needed nor useful, so defaulting to -M might be sufferable.

(And you'd probably also have to support -FM ..., and ignore -F.)

-FM sets the output filename to "M", standalone -F will not require one of -M and -MM. if there is no -MM, it should just be equivalent to -MF...

(I'm getting closer, thanks for your help)

comment:10 follow-ups: Changed 8 years ago by ohanar

  • Cc robertwb added

Since this is an upstream feature, I think it would be better to submit a pull request at Cython's github with a cleaned-up version of your patch. Really, we should only patch cython for bug fixes or for features not included in the latest release that we want/need.

comment:11 in reply to: ↑ 10 Changed 8 years ago by leif

Replying to ohanar:

Since this is an upstream feature, I think it would be better to submit a pull request at Cython's github with a cleaned-up version of your patch.

I was assuming that's exactly what Felix was finally going to do (once the patch is in a submittable state); at least the "Report upstream" field is already set...

comment:12 in reply to: ↑ 10 Changed 8 years ago by felixs

Replying to ohanar:

Since this is an upstream feature, I think it would be better to submit a pull request at Cython's github with a cleaned-up version of your patch. Really, we should only patch cython for bug fixes or for features not included in the latest release that we want/need.

Its a bugfix, it's not included in the latest release and I want/need it. I've just added a -D switch and noticed cython-devel.

comment:13 follow-up: Changed 8 years ago by ohanar

Considering it is marked 'wishlist' upstream, they definitely consider it a feature, not a bug. Also, at least from my perspective (correct me if I'm wrong Robert), but it seems like using the cython command line utility is not the recommended way to use cython -- rather it seems that cythonize+distutils/setuptools/distribute seems like the recommended way going forward. So I imagine adding this functionality to cython is not even on the radar for upstream. To avoid bitrot, I would seriously suggest submitting something upstream at least to get the ball rolling.

comment:14 in reply to: ↑ 13 ; follow-up: Changed 8 years ago by leif

Replying to ohanar:

Considering it is marked 'wishlist' upstream, they definitely consider it a feature, not a bug. Also, at least from my perspective (correct me if I'm wrong Robert), but it seems like using the cython command line utility is not the recommended way to use cython -- rather it seems that cythonize+distutils/setuptools/distribute seems like the recommended way going forward.

Well, it's perhaps Cython upstream's "recommended way", but in fact it is currently the only way because of the missing feature this ticket is supposed to add... ;-)

comment:15 Changed 8 years ago by jondo

  • Cc jondo added

comment:16 Changed 8 years ago by jdemeyer

  • Milestone changed from sage-5.11 to sage-5.12

comment:17 in reply to: ↑ 14 Changed 8 years ago by felixs

  • Branch set to u/felixs/cython_deps
  • Commit set to bd11c246d0df90dac0fa2f362c6d4e961cece5ad

It might be just 'wishlist' upstream. Since it breaks anything that requires plain dependencies for cython generated files, and since there is no better option in the dropdown, i'll leave it like this.

(The patch has moved to git, it looks like i cannot remove the attached file)

comment:18 Changed 8 years ago by felixs

  • Status changed from new to needs_review

comment:19 Changed 8 years ago by vbraun_spam

  • Milestone changed from sage-6.1 to sage-6.2

comment:20 Changed 7 years ago by vbraun_spam

  • Milestone changed from sage-6.2 to sage-6.3

comment:21 Changed 7 years ago by vbraun_spam

  • Milestone changed from sage-6.3 to sage-6.4

comment:22 Changed 6 years ago by chapoton

  • Status changed from needs_review to needs_work

cython patches do not apply

comment:23 Changed 4 years ago by jdemeyer

  • Authors Felix Salfelder deleted
  • Milestone changed from sage-6.4 to sage-duplicate/invalid/wontfix
  • Reviewers set to Jeroen Demeyer
  • Status changed from needs_work to positive_review

Given that upstream Cython has no plans for this and given that nobody uses automake for Python projects, I suggest to close this as "wontfix".

comment:24 Changed 4 years ago by embray

  • Resolution set to wontfix
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.