Page MenuHomeFreeBSD

Bump LLVM_DEFAULT to 90
ClosedPublic

Authored by jbeich on Aug 6 2019, 8:35 PM.

Details

Test Plan

poudriere bulk -t is green on 11.2 amd64

Diff Detail

Repository
rP FreeBSD ports repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

jbeich edited the test plan for this revision. (Show Details)
jbeich added a reviewer: tobik.
jbeich added a subscriber: tobik.

@tobik, can you review how LLVM_DEFAULT is modified?
8/36 aka 22% of consumers aren't compatible:

tobik requested changes to this revision.Aug 7 2019, 8:20 AM
tobik added inline comments.
security/afl++/Makefile
51–53 ↗(On Diff #60532)

I think it is kind of ugly and confusing to set LLVM_DEFAULT like this. Ports in general should not set any *_DEFAULT.

Anyway it is not right with DEFAULT_VERSIONS+=llvm=-devel:

make: "/home/tobias/ports/llvm/security/afl++/Makefile" line 52: warning: String comparison operator should be either == or !=
make: "/home/tobias/ports/llvm/security/afl++/Makefile" line 52: Malformed conditional (${LLVM_DEFAULT} >= 90 || ${LLVM_DEFAULT} == "-devel")
make: Fatal errors encountered -- cannot continue
make: stopped in /home/tobias/ports/llvm/security/afl++

You could swap the comparisons, string first then integer, which solves the errors but will still show the warnings. Maybe do it like this instead

.if ${LLVM_DEFAULT:S,-devel,100,} >= 90
LLVM_DEFAULT=	80
.endif

As for afl++ and ccls could you share the failure logs so that I can send them upstream?

textproc/castxml/Makefile
32 ↗(On Diff #60532)

LLVM_40 is stale. devel/llvm40 was removed a while ago.

This revision now requires changes to proceed.Aug 7 2019, 8:20 AM
devel/ccls/Makefile
42–47 ↗(On Diff #60532)

Should no longer be needed after rP508301.

security/afl/Makefile
7 ↗(On Diff #60532)

security/afl is not ready as even a basic make test fails. It would probably be best to lock it to llvm80 too.

jbeich marked 2 inline comments as done.
  • Unbreak DEFAULT_VERSIONS+=llvm=-devel support
  • Rebase after rP508301 and rP508303
security/afl++/Makefile
51–53 ↗(On Diff #60532)

Ports in general should not set any *_DEFAULT.

Short of introducing USES=llvm (a la USES=python) there's no other way to limit range.

.if ${LLVM_DEFAULT:S,-devel,100,} >= 90

-devel is supposed to be always greater, so I've used 990. Given current cadence of 2 major releases per year it won't need to be changed for ~45 years. ;)

As for afl++ and ccls could you share the failure logs so that I can send them upstream?

See P287 and P288 (previous version).

security/afl++/Makefile
51–53 ↗(On Diff #60532)
  • Per @tobik: limit security/afl to llvm < 90 due to make test failure

@jmd or @zeising, can you update devel/libclc to a more recent LLVM snapshot? lang/clover (P294) compiles fine against LLVM 9 API while linking errors maybe unrelated.

@jmd or @zeising, can you update devel/libclc to a more recent LLVM snapshot? lang/clover (P294) compiles fine against LLVM 9 API while linking errors maybe unrelated.

https://reviews.freebsd.org/D21019

This revision is now accepted and ready to land.Aug 7 2019, 11:23 AM

@jbeich Any idea what is going on with P294 and the spam in the Phabricator feed? It happens every 5 minutes or so. Some automation gone awry maybe?

@tobik, still no clue what you mean. I only get self-action spam despite turning it off in phabricator settings.

@tobik, still no clue what you mean. I only get self-action spam despite turning it off in phabricator settings.

See https://reviews.freebsd.org/feed/ and the bazillion "jbeich edited P294" messages. But I'm guessing this is something for the Phabricator admins to fix.

security/afl++/Makefile
51–53 ↗(On Diff #60540)

This can be removed again after rP508373.

This revision now requires review to proceed.Aug 8 2019, 12:45 PM

Rather than overriding the global variable LLVM_DEFAULT I would prefer to see the following usage:

Index: devel/ispc/Makefile

  • devel/ispc/Makefile (revision 507746)

+++ devel/ispc/Makefile (working copy)
@@ -15,13 +15,14 @@
ONLY_FOR_ARCHS= amd64 i386
ONLY_FOR_ARCHS_REASON= only available for x86 architectures

-LIB_DEPENDS= libLLVM.so:devel/llvm${LLVM_DEFAULT}
+LLVM_VER= ${LLVM_DEFAULT}
+LIB_DEPENDS= libLLVM.so:devel/llvm${LLVM_VER}

USES= bison cmake python:build shebangfix
USE_GITHUB= yes
SHEBANG_FILES= *.py

-CONFIGURE_ENV= PATH=${LOCALBASE}/llvm${LLVM_DEFAULT}/bin:${PATH}
+CONFIGURE_ENV= PATH=${LOCALBASE}/llvm${LLVM_VER}/bin:${PATH}
CMAKE_OFF= ISPC_INCLUDE_EXAMPLES

BINARY_ALIAS= python=${PYTHON_CMD}
@@ -41,4 +42,10 @@

./${e}

.endfor

+.include <bsd.port.options.mk>
+
+.if ${LLVM_DEFAULT:S,-devel,990,} >= 90
+LLVM_VER= 80
+.endif
+
.include <bsd.port.mk>

If desired, I can apply this patch to the 5 ports involved and re-upload.

For related work, please see the wiki page https://wiki.freebsd.org/HardcodedLLVMVersions, in particular, case 1.

If desired, I can apply this patch to the 5 ports involved and re-upload.

  • If an extra variable is necessary it should be in all consumers, not busted only
  • LLVM_DEFAULT compatibility is restricted here only temporary, until fixes land
  • During llvm100 switch (around 2020Q1) different consumers will likely break

Again, until USES=llvm or similar is introduced let's avoid churn. Some other *_DEFAULT variables also lack USE or USES e.g. JULIA_DEFAULT and RUST_DEFAULT.

  • If an extra variable is necessary it should be in all consumers, not busted only

I have local patches to do this. But IMHO fixing these cases should come first.

  • LLVM_DEFAULT compatibility is restricted here only temporary, until fixes land

I don't understand this statement.

  • During llvm100 switch (around 2020Q1) different consumers will likely break

I don't understand why this should mean this change not go in.

Again, until USES=llvm or similar is introduced let's avoid churn.

Adding the USES=llvm code is a worthy goal but I don't have the cycles to do it ATM. These fixes seem to me to be needed in the meantime.

Some other *_DEFAULT variables also lack USE or USES e.g. JULIA_DEFAULT and RUST_DEFAULT.

So? There's only so many things I can try to fix at one time.

arrowd added inline comments.
security/klee/Makefile
53 ↗(On Diff #60997)

Why not hardcode 80 here, instead of using LLVM_DEFAULT?

  • If an extra variable is necessary it should be in all consumers, not busted only

I have local patches to do this. But IMHO fixing these cases should come first.

Inconsistency complicates maintenance. If all LLVM_DEFAULT consumers look the same it's easy to refactor.

  • LLVM_DEFAULT compatibility is restricted here only temporary, until fixes land

I don't understand this statement.

A number of consumers have already been fixed before the patch here landed. Dropping hacks involves removing a few lines instead of renaming LLVM_VER back to LLVM_DEFAULT.

Anyway, if you consider direct usage of LLVM_DEFAULT as hardcoded then just do the cleanup (or open separate differential for review) without trying to hijack an unrelated change. Make it atomic to minimize rebase churn and facilitate post-commit review. I'm not particularly attached to the current style.

security/klee/Makefile
53 ↗(On Diff #60997)

To keep support for older devel/llvm* e.g., DEFAULT_VERSIONS+=llvm=60.

jbeich marked an inline comment as done.
  • Rebase
This revision was not accepted when it landed; it landed in state Needs Review.Sep 20 2019, 7:58 PM
This revision was automatically updated to reflect the committed changes.