poudriere bulk -t is green on 11.2 amd64
Details
Details
- Reviewers
tobik - Group Reviewers
O5: Ports Framework portmgr - Commits
- rP512440: Switch default devel/llvm* to 90
Diff Detail
Diff Detail
- Repository
- rP FreeBSD ports repository
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
Comment Actions
@tobik, can you review how LLVM_DEFAULT is modified?
8/36 aka 22% of consumers aren't compatible:
| 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
.endifAs 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. |
| security/afl++/Makefile | ||
|---|---|---|
| 51–53 ↗ | (On Diff #60532) |
Short of introducing USES=llvm (a la USES=python) there's no other way to limit range.
-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. ;)
|
| security/afl++/Makefile | ||
|---|---|---|
| 51–53 ↗ | (On Diff #60532) | Thanks, reported here: https://github.com/vanhauser-thc/AFLplusplus/issues/40 |
Comment Actions
@tobik, still no clue what you mean. I only get self-action spam despite turning it off in phabricator settings.
Comment Actions
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.
Comment Actions
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.
Comment Actions
- 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.
Comment Actions
- 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.
| security/klee/Makefile | ||
|---|---|---|
| 53 ↗ | (On Diff #60997) | Why not hardcode 80 here, instead of using LLVM_DEFAULT? |
Comment Actions
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. |