Skip to content

Conversation

hartbit
Copy link
Contributor

@hartbit hartbit commented Nov 26, 2018

This only applies to Clang targets. Swift targets will be fixed in llbuild separately, but the tests have been modified in preperation.

@aciidgh
Copy link
Contributor

aciidgh commented Nov 27, 2018

I don't think we want to change this. It is consistent with Xcode's log reporting as well.

@hartbit
Copy link
Contributor Author

hartbit commented Nov 27, 2018

But it’s inconsistent in tense to the « Linking » tool and to Ninja’s « Building » messages.

@aciidgh
Copy link
Contributor

aciidgh commented Nov 27, 2018

Maybe we should rename Linking.. to Link.. then?

@hartbit
Copy link
Contributor Author

hartbit commented Nov 27, 2018

Makes less sense to me, but I'll bow down to whatever decision is taken.

@aciidgh
Copy link
Contributor

aciidgh commented Nov 27, 2018

I don't remember the exact reason for this wording but maybe @ddunbar does?

@ddunbar
Copy link
Contributor

ddunbar commented Nov 27, 2018

We definitely should be consistent.

I think Xcode's log may be different, because it is usually viewed after the fact, so its not actually compiling any more. The running status in the console is more often viewed as a status of the progress, but not a persistent artifact in a log (even though it does appear as both).

I'd probably use "Compiling", but don't have a strong opinion other than that we should be consistent.

@aciidgh
Copy link
Contributor

aciidgh commented Nov 27, 2018

ok, then I am also fine with anything.

@dmbryson
Copy link
Contributor

I definitely agree that we should be consistent, whichever wording is chosen. I'd give a slight nod toward the existing 'compile' phrasing, giving a +1 to Ankit's comment about changing 'Linking' to 'Link'. When stated as 'compiling' it feels like it should be followed by a 'done' statement somewhere, which is complexity that I am not sure is really necessary.

@hartbit
Copy link
Contributor Author

hartbit commented Nov 27, 2018

I originally went with Compiling because:

  • I'm getting quite used to and liking NInja's Building ...
  • Compile/Link, as verbs, feel more like actions to be taken, not status updates

@jakepetroules
Copy link
Contributor

As usual, consistency trumps everything, but I lean slightly towards the "summary of action" phrasing that we currently use in Xcode, for example "Compile Foo.c", "Link Bar".

Status in a progress bar can be reported separately of the phrasing of the action itself. For example, I've seen build tools use Unicode symbols to indicate this in a rather nice way (I might not have picked the right characters that have metrics consistent to each other in whatever font they'll show up with, but you get the point):

  • ○ Compile Foo.c (in progress)
  • ✓ Compile Foo.c (done, success)
  • ✗ Compile Foo.c (done, error)

Otherwise, I think you have to argue that the phrasing itself should change depending on how the log is being viewed. If "Compiling Foo.c" shows up in a log after the build has finished, it reads awkwardly. Similarly, "Compiled Foo.c" doesn't work for an in-progress build. Logs won't always be displayed in an environment where their content can be dynamically updated (like the Xcode build log or Ninja progress thingy) -- they have to be committed to static files eventually.

"Compile Foo.c" is more grammatically neutral, and can read well in past, present, and future tenses.

@hartbit
Copy link
Contributor Author

hartbit commented Nov 28, 2018

"Compile Foo.c" is more grammatically neutral, and can read well in past, present, and future tenses.

While I understand the logic, Compile feels slightly weird to me. Also, most of the build systems out there use the -ing tense:

How do we decide?

@aciidgh
Copy link
Contributor

aciidgh commented Nov 30, 2018

Let's take this change.

@aciidgh
Copy link
Contributor

aciidgh commented Nov 30, 2018

@swift-ci smoke test

@aciidgh aciidgh merged commit 45befd5 into swiftlang:master Nov 30, 2018
@hartbit hartbit deleted the compile-to-compiling branch December 8, 2018 13:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants