Page MenuHomePhabricator

Remove __VERSION__
AbandonedPublic

Authored by sylvestre.ledru on Jul 2 2019, 12:35 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

Actually, I don't think removing -dumpversion is a great idea. it will remove the capability of clang to be a dropped in replacement.

I see a lot of occurrences:
https://github.com/search?q=%22-dumpversion%22&type=Code
https://codesearch.debian.net/search?q=%22-dumpversion%22

(just in the source of Firefox, I see 6 different usage)

rnk added a comment.Jul 2 2019, 1:52 PM

But, based on the github search, I don't think we could reasonably change dumpversion to print 8.0.0, so I'm not sure it really matters. Firefox builds with clang these days, so I'm not sure what they could possibly be using -dumpversion for that would matter.

This isn't firefox per say but thirdparty apps.
If you feel confident, sure, we can land that and see what happens :)

@rnk is it good for you ?

rnk added a comment.Jul 8 2019, 5:30 PM

This isn't firefox per say but thirdparty apps.
If you feel confident, sure, we can land that and see what happens :)

One concern that I have is that we don't have a replacement for -dumpversion. I thought I saw someone propose adding such a flag, but I can't find it. Basically, we should do something to handle this user's use case:
http://clang-developers.42468.n3.nabble.com/How-to-dump-clang-version-td4025542.html

docs/ReleaseNotes.rst
83 ↗(On Diff #207481)

Maybe "It had been introduced..." or "It was introduced..." instead.

Actually, I think it's worth taking the time to elaborate a bit. Users *will* trip over this, and it's likely that the release notes will become the canonical documentation for it.

Try this text:

- The ``-dumpversion`` flag and the ``__VERSION__`` macro have been removed.
  Previously they both produced information designed for compatibility with GCC
  4.2.1, but that should no longer be necessary. To get clang's version, use the
  `clang namespaced version macros`_ and ``--version``.

_ https://clang.llvm.org/docs/LanguageExtensions.html#builtin-macros

Update of the release note.
thanks to @rnk for the suggestion

sylvestre.ledru marked an inline comment as done.Jul 9 2019, 2:25 AM

@rnk Works for you?
I would like to land that before the 9 branch. Thanks

rnk added a comment.Jul 11 2019, 12:57 PM

I still have this open concern:

In D64062#1574868, @rnk wrote:

One concern that I have is that we don't have a replacement for -dumpversion. I thought I saw someone propose adding such a flag, but I can't find it. Basically, we should do something to handle this user's use case:
http://clang-developers.42468.n3.nabble.com/How-to-dump-clang-version-td4025542.html

Perhaps we should just remove __VERSION__ and keep -dumpversion.

In D64062#1581290, @rnk wrote:

Perhaps we should just remove __VERSION__ and keep -dumpversion.

As you wish. clang --version seems an alternative and dumpversion never worked for clang. so, I don't expect software to use it for clang.

rnk added a comment.Jul 11 2019, 4:18 PM
In D64062#1581290, @rnk wrote:

Perhaps we should just remove __VERSION__ and keep -dumpversion.

As you wish. clang --version seems an alternative and dumpversion never worked for clang. so, I don't expect software to use it for clang.

That's true, but users seem to want some programmatic way to extract the compiler version number without having to resort to sed & co, and we don't provide that right now. -dumpversion can easily be the name for that feature.

Keep -dumpversion

Actually remove it

sylvestre.ledru retitled this revision from Remove both -dumpversion and __VERSION__ to Remove __VERSION__.Jul 12 2019, 6:42 AM
sylvestre.ledru edited the summary of this revision. (Show Details)

@rnk Should be good this time :)

rnk accepted this revision.Jul 12 2019, 11:50 AM

lgtm, minor changes

docs/ReleaseNotes.rst
82 ↗(On Diff #209474)

I guess move the bullet to the generic "list of changes", since this isn't a flag or option.

85 ↗(On Diff #209474)

typo "The"

86–87 ↗(On Diff #209474)

"Previously this macro was set to a string aiming to achieve compatibility with GCC 4.2.1, but that should no longer be necessary. To get Clang's version..."

This revision is now accepted and ready to land.Jul 12 2019, 11:50 AM
sylvestre.ledru marked 3 inline comments as done.Jul 12 2019, 2:43 PM
Closed by commit rL365962: Remove __VERSION__ (authored by sylvestre, committed by ). · Explain WhyJul 12 2019, 2:45 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJul 12 2019, 2:45 PM

This revision breaks python 2.7.16 builds, which are still supported by upstream python for a few more months. I'm preparing a revert.

The file is getcompiler.c:

/* Return the compiler identification, if possible. */

#include "Python.h"

#ifndef COMPILER

#ifdef GNUC
#define COMPILER "\n[GCC " VERSION "]"
#endif

#endif /* !COMPILER */

#ifndef COMPILER

#ifdef __cplusplus
#define COMPILER "[C++]"
#else
#define COMPILER "[C]"
#endif

#endif /* !COMPILER */

const char *
Py_GetCompiler(void)
{

return COMPILER;

}

The revert is at https://reviews.llvm.org/D64687, which I will commit shortly if there are no objections.

sylvestre.ledru reopened this revision.Jul 12 2019, 11:31 PM

@rnk looks like it is too risky to remove it.
What about changing it from

#define __VERSION__ "4.2.1 Compatible Clang 7.0.1 (tags/RELEASE_701/final)"

to

#define __VERSION__ "Clang 7.0.1 (tags/RELEASE_701/final)"

Hopefully, it won't break much stuff

This revision is now accepted and ready to land.Jul 12 2019, 11:31 PM

From the failure mode I was seeing, any value at all for VERSION will be fine, as long as it is a valid preprocessor string.

It would also solve this particular problem to not define GNUC, but I as that would create other, more severe, problems, as long as clang identifies itself as GNUC, we should maintain compatibility with it.

sylvestre.ledru requested review of this revision.Jul 14 2019, 1:40 AM
rnk added a comment.Jul 15 2019, 10:38 AM

This revision breaks python 2.7.16 builds, which are still supported by upstream python for a few more months. I'm preparing a revert.

The file is getcompiler.c:

I just want to point out that this code was already doing the wrong thing. It will report that the compiler is some incorrect version of GCC. After we add VERSION back it will keep doing the wrong thing, which was sort of the point of removing it in the first place: to find such broken code and fix it. In any case, it sounds like it's not worth the effort, so let's just change the value of VERSION instead of removing it.

sylvestre.ledru abandoned this revision.Jul 15 2019, 10:47 AM