Skip to content

Fix #1735: Discover and use clang-10 or clang-9, if present. #1736

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
May 15, 2020

Conversation

LeeTibbert
Copy link
Contributor

@LeeTibbert LeeTibbert commented Feb 29, 2020

  • clang-10 and clang-9 are now available. Discover and use the highest
    installed clang on the build system. This is the next
    in a long series of clang update PRs. It supersedes pending PR Discover & use clang-9, if present. #1686.

  • Scala Native appears to build and run test-all without notice using
    either clang-10 or clang-9. The only difference is the audible
    sizzle of Cool.

  • I removed the line ("8", "0") because a year has gone by and shown that
    bit of defensive programming to be unneeded. It has served its purpose.
    The resultant code will contain less lore and execute slightly quicker.
    clang-8 will still be discovered, if it is the highest installed version.

  • The Discovery code does not, and has not for years, discover
    llvm-config of the form llvm-config-N, where N is [3-9]. The current
    PR does no worse that prior art. A PR for another day.

  • This PR adds two clang version to the current discovery mechanism.
    That mechanism is now checking quite a load of ancient clang versions.

    I believe that the discovery mechanism should be changed to a
    "specification" mechanism. That is, require developers to
    specify any desired clang version other than the system default "clang".
    I did not make this change to minimize churn whilst ScalaNative is
    undergoing re-organization.

Documentation:

* The standard changelog entry is requested.

Testing:

  • Testing needs to show first safety, then efficacy.

  • Safety, not breaking when neither clang-10 nor clang-9 is installed,
    will be shown by Travis CI.

  • To show that the code change was effective on a system with both
    clang-10 and clang-9 installed, I manually added
    'nativeCompileOptions ++= Seq("-v")'to the settings in build.sbt for
    the sandbox project and then running that project.

    The log file showed clang-10 in use.

    I then ran "test-all" using clang-10 and all tests passed.

  • I manually tested as above on the same system with only clang-9 (and
    clang itself) installed. clang-9 was used and all tests passed.

@LeeTibbert
Copy link
Contributor Author

@ekrich left the comment below in the now superseded PR #1686. That comment may be relevant to this PR.

I think we need this for the new version of Metals which seems to check using Discover.clang() or clangPP() and gives an error if using clang version 9.0.0.

@sjrd
Copy link
Collaborator

sjrd commented May 15, 2020

Could you rebase this on top of the latest master to make sure scalafmt still passes? Thank you.

@sjrd sjrd changed the title Fix #1735: SN build should Discover & use clang-10 or clang-9, if present Fix #1735: Discover and use clang-10 or clang-9, if present. May 15, 2020
…ng-9, if present.

  * clang-10 and clang-9 are now available. Discover and use the highest
    installed clang on the build system.  This is the next
    in a long series of clang update PRs. It supersedes pending PR scala-native#1686.

  * Scala Native appears to build and run test-all without notice using
    either clang-10 or clang-9.  The only difference is the audible
    sizzle of Cool.

  * The Discovery code does not, and has not for years, discover
    llvm-config of the form llvm-config-N, where N is [3-9]. The current
    PR does no worse that prior art.  A PR for another day.

  * This PR adds two clang version to the current discovery mechanism.
    That mechanism is now checking quite a load of ancient clang versions.

    I believe that the discovery mechanism should be changed to a
    "specification" mechanism.  That is, require developers to
    specify any desired clang version other than the system default "clang".
    I did not make this change to minimize churn whilst ScalaNative is
    undergoing re-organization.

Documentation:

    * The standard changelog entry is requested.

Testing:

  * Testing needs to show first safety, then efficacy.

  * Safety, not breaking when neither clang-10 nor clang-9 is installed,
    will be shown by Travis CI.

  * To show that the code change was effective on a system with both
    clang-10 and clang-9 installed, I manually added
    'nativeCompileOptions ++= Seq("-v")'to the settings in build.sbt for
    the sandbox project and then running that project.

    The log file showed clang-10 in use.

    I then ran "test-all" using clang-10 and all tests passed.

  * I manually tested as above on the same system with only clang-9 (and
    clang itself) installed.  clang-9 was used and all tests passed.
@LeeTibbert LeeTibbert force-pushed the PR_build_Discover_clang-10 branch from 0482c14 to c5bce7a Compare May 15, 2020 16:30
@LeeTibbert
Copy link
Contributor Author

rebased & Travis CI is green across the board.

I use clang-10 pretty intensively in my daily Scala Native work and have
done so for a month or so. No problems seen.

@sjrd sjrd merged commit b603b44 into scala-native:master May 15, 2020
ekrich pushed a commit to ekrich/scala-native that referenced this pull request May 21, 2021
…nt. (scala-native#1736)

Scala Native appears to build and run test-all without notice using
either clang-10 or clang-9.  The only difference is the audible
sizzle of Cool.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants