Bug 212334 - devel/llvm-devel: Improve port (v4.0.d20161115)
Summary: devel/llvm-devel: Improve port (v4.0.d20161115)
Status: Closed FIXED
Alias: None
Product: Ports & Packages
Classification: Unclassified
Component: Individual Port(s) (show other bugs)
Version: Latest
Hardware: Any Any
: --- Affects Some People
Assignee: Brooks Davis
URL:
Keywords: patch
Depends on:
Blocks:
 
Reported: 2016-09-02 15:31 UTC by lightside
Modified: 2017-01-16 03:24 UTC (History)
2 users (show)

See Also:
bugzilla: maintainer-feedback? (brooks)


Attachments
Proposed patch (since 421246 revision) (147.61 KB, patch)
2016-09-02 15:31 UTC, lightside
no flags Details | Diff
Proposed patch (since 421246 revision) (147.62 KB, patch)
2016-09-02 15:43 UTC, lightside
no flags Details | Diff
Proposed patch (since 421246 revision) (147.50 KB, patch)
2016-09-02 15:56 UTC, lightside
no flags Details | Diff
Proposed patch (since 421246 revision) (147.52 KB, patch)
2016-09-03 16:01 UTC, lightside
no flags Details | Diff
Proposed patch (since 421246 revision, second variant) (27.75 KB, patch)
2016-09-03 16:21 UTC, lightside
no flags Details | Diff
Proposed patch (since 421246 revision, second variant) (27.75 KB, patch)
2016-09-03 16:33 UTC, lightside
no flags Details | Diff
Proposed patch (since 421246 revision, third variant) (25.76 KB, patch)
2016-09-03 17:00 UTC, lightside
no flags Details | Diff
Proposed patch (since 421246 revision, third variant) (25.67 KB, patch)
2016-09-30 23:01 UTC, lightside
no flags Details | Diff
Proposed patch (since 423371 revision) (24.09 KB, patch)
2016-10-07 11:49 UTC, lightside
no flags Details | Diff
Proposed patch (since 423371 revision) (24.09 KB, patch)
2016-10-07 11:59 UTC, lightside
no flags Details | Diff
Proposed patch (since 423371 revision) (24.21 KB, patch)
2016-10-07 13:00 UTC, lightside
no flags Details | Diff
Proposed patch (since 423371 revision) (24.20 KB, patch)
2016-10-07 13:13 UTC, lightside
no flags Details | Diff
Proposed patch (since 424411 revision) (24.19 KB, patch)
2016-10-25 04:42 UTC, lightside
no flags Details | Diff
Proposed patch (since 426205 revision) (25.34 KB, patch)
2016-11-16 13:00 UTC, lightside
no flags Details | Diff
Proposed patch (since 426205 revision) (26.16 KB, patch)
2016-11-16 13:06 UTC, lightside
no flags Details | Diff
The devel/llvm-devel port (v4.0.d20161115) with proposed changes (22.16 KB, application/x-bzip2)
2016-11-16 13:37 UTC, lightside
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description lightside 2016-09-02 15:31:55 UTC
Created attachment 174316 [details]
Proposed patch (since 421246 revision)

Patch to improve devel/llvm-devel port.

- Bump PORTREVISION
- Add license information
- Use VAR option helper for _USES_PYTHON variable
- Simplify PORTDOCS with * value
- Add UTILS (default) option to install utility binaries (e.g. count, FileCheck, not, etc.)
- Change description for LIT option
- Add dependency on UTILS for LIT option
- Use new GH_SUBDIR option helper, instead of post-extract-* targets
- Add libxml2 to USE_GNOME for CLANG option, because c-index-test requires this, according to stage-qa
- Sort variables for options (e.g. GOLD_*) and change placement of some variables
- Remove bsd.port.options.mk include
- Add changes to .MAKEFLAGS in case of makesum target to fetch all optional distfiles for distinfo file creation, independent from selected options
- Replace blank space(s) with tab for some cases
- Build compiler-rt as a project in projects/compiler-rt directory
- Remove COMMANDS variable, check-commands target and simplify llvm-wrapper.sh installation in post-install-script target
- Replace contents of build-plist target with dynamic approach and POST_PLIST usage
- Remove regression-test target; use new TEST_TARGET and TEST_WRKSRC variables for `make test`
- Remove post-patch target for lit changes, which allows usage of testing targets
- Install lit with using utils/lit/setup.py; add Python setuptools to build dependency for LIT option
- Remove OPTIONS_SUB, PLIST_FILES and pkg-plist file
- Remove curl and jq requirement with using available commands in files/gen-Makefile.snapshot.sh script
- Add files/llvm-lit-wrapper.sh.in, which contains PYTHONPATH environment variable, for correct (llvm-)lit${LLVM_SUFFIX} installation and usage

The build tested on FreeBSD 10.2 amd64.
Comment 1 lightside 2016-09-02 15:34:01 UTC
I set TEST_TARGET to "check" value (TEST_TARGET=check), which does LLVM regression tests by default.
Possible to set TEST_TARGET to "check-all" value, to run available tests, depending on selected options for port. But I tried this and some (sanitizer) test didn't stop, so I reverted it to "check" value.

Example of output for LLVM regression tests (TEST_TARGET=check):
-8<--
% make test
...
[0/1] Running the LLVM regression tests
Testing Time: 474.86s
  Expected Passes    : 17246
  Expected Failures  : 125
  Unsupported Tests  : 372
-->8-

Example of output for Clang regression tests:
-8<--
% make TEST_TARGET=check-clang test
...
[0/1] Running the Clang regression tests
...
********************
Testing Time: 310.77s
********************
Failing Tests (284):
...
  Expected Passes    : 9410
  Expected Failures  : 16
  Unsupported Tests  : 42
  Unexpected Failures: 284
...
-->8-

Example of output for Clang extra tools' regression tests:
-8<--
% make TEST_TARGET=check-clang-tools test
...
[0/1] Running the Clang extra tools' regression tests
Testing Time: 28.73s
  Expected Passes    : 371
-->8-

Results for v4.0.d20160824 on FreeBSD 10.2 amd64.
Comment 2 lightside 2016-09-02 15:43:53 UTC
Created attachment 174317 [details]
Proposed patch (since 421246 revision)

Placed .ifmake makesum check before bsd.port.pre.mk include.
Comment 3 lightside 2016-09-02 15:56:36 UTC
Created attachment 174318 [details]
Proposed patch (since 421246 revision)

(In reply to comment #0)
> - Add changes to .MAKEFLAGS in case of makesum target to fetch
> all optional distfiles for distinfo file creation, independent
> from selected options
Looks like, better to remove this part (for now), because of OPTIONS_DEFINE_* for other architectures.
Comment 4 Brooks Davis freebsd_committer 2016-09-02 16:06:49 UTC
Thank you for your submission.  Many of these changes look like improvements, but some I do not like.  Having an enormous patch full of unrelated changes makes it hard to separate them.

For now, I reject these two changes:
> - Remove COMMANDS variable, check-commands target and simplify llvm-wrapper.sh installation in post-install-script target
> - Replace contents of build-plist target with dynamic approach and POST_PLIST usage

In order to eventually support multiple packages per port, we need to track which parts go with which component.  Since that feature is essential for the continued improvement of the system, I'd rather not throw away all the related information to save a little maintenance burden now.
Comment 5 lightside 2016-09-02 16:37:36 UTC
(In reply to comment #4)
> Thank you for your submission.
Thanks for your attention. I proposed some of these changes privately in 2015 year, as you may remember.

(In reply to comment #4)
> Having an enormous patch full of unrelated changes makes it hard to separate
> them.
Most size of the patch is pkg-plist removal.
Other parts were explained in description of comment #0.

(In reply to comment #4)
> In order to eventually support multiple packages per port, we need to track
> which parts go with which component. Since that feature is essential for the
> continued improvement of the system, I'd rather not throw away all the related
> information to save a little maintenance burden now.
Following command gives the same information after stage:
% make -V TMPPLIST | xargs cat

In my opinion, your decision to go with static pkg-plist (but dynamic nature of it) limits usage of the port by other people, who may update the port for newer version(s).

Many of proposed changes applies to other devel/llvm* ports, because of remained errors (e.g., in COMMANDS variable with missing value(s)), including correct lit testing tool usage (e.g. necessity of utils/lit/lit/formats directory to install).

In other words, include good changes what you see fit, while I agree with my opinion.
Comment 6 commit-hook freebsd_committer 2016-09-03 00:21:09 UTC
A commit references this bug:

Author: brooks
Date: Sat Sep  3 00:20:06 UTC 2016
New revision: 421280
URL: https://svnweb.freebsd.org/changeset/ports/421280

Log:
  Upgrade to 3.9.0 release.

  Improve support for -fopenmp with a hack inspired by a submission from
  Johannes Dieterich <dieterich@ogolem.org>.

  Implement a number of improvments submitted by lightside@gmx.com:
   - Add license information
   - Use VAR option helper for _USES_PYTHON variable
   - Add libxml2 to USE_GNOME for CLANG option
   - Sort variables for options (e.g. GOLD_*)

  Add USES=execinfo for LLDB.

  PR:		203223, 212334

Changes:
  head/devel/llvm39/Makefile
  head/devel/llvm39/distinfo
  head/devel/llvm39/files/clang-patch-fopenmp.diff
Comment 7 lightside 2016-09-03 08:07:57 UTC
(In reply to comment #4)
> For now, I reject these two changes:
>> - Remove COMMANDS variable, check-commands target and simplify
>> llvm-wrapper.sh installation in post-install-script target
>> - Replace contents of build-plist target with dynamic approach and
>> POST_PLIST usage
I will try to change the proposed patch to use COMMANDS variable and pkg-plist file. This requires some time, including for testing build with all options.
Comment 8 lightside 2016-09-03 16:01:07 UTC
Created attachment 174339 [details]
Proposed patch (since 421246 revision)

Added COMPILER_RT_IMPLIES=CLANG, because the compiler-rt requires clang and clang-headers targets, when build as a project in projects/compiler-rt directory.
Comment 9 lightside 2016-09-03 16:21:25 UTC
Created attachment 174340 [details]
Proposed patch (since 421246 revision, second variant)

Attached second variant, which uses COMMANDS variable and pkg-plist, with following overall changes:

- Bump PORTREVISION
- Add license information
- Use VAR option helper for _USES_PYTHON variable
- Simplify PORTDOCS with * value
- Add UTILS (default) option to install utility binaries (e.g. count, FileCheck, not, etc.)
- Change description for LIT option
- Add dependency on UTILS for LIT option
- Use new GH_SUBDIR option helper, instead of post-extract-* targets
- Add libxml2 to USE_GNOME for CLANG option, because c-index-test requires this, according to stage-qa
- Sort variables for options (e.g. GOLD_*) and change placement of some variables
- Remove second OPTIONS_SUB definition
- Replace blank space(s) with tab for some cases
- Build compiler-rt as a project in projects/compiler-rt directory; add dependency on CLANG for COMPILER_RT option
- Move installation of llvm-wrapper.sh to post-install-script target
- Remove macho-dump from COMMANDS and clang-modernize from EXTRAS_COMMANDS, which checked with check-commands target
- Remove regression-test target; use new TEST_TARGET and TEST_WRKSRC variables for `make test`
- Remove post-patch target for lit changes, which allows usage of testing targets
- Install lit with using utils/lit/setup.py; add Python setuptools to build dependency for LIT option
- Adapt build-plist target
- Remove curl and jq requirement with using available commands in files/gen-Makefile.snapshot.sh script
- Add files/llvm-lit-wrapper.sh.in, which contains PYTHONPATH environment variable, for correct (llvm-)lit${LLVM_SUFFIX} installation and usage
- Adapt pkg-plist

and other small changes for variables.
Comment 10 lightside 2016-09-03 16:33:25 UTC
Created attachment 174344 [details]
Proposed patch (since 421246 revision, second variant)

Cosmetic fix.
Comment 11 lightside 2016-09-03 16:41:24 UTC
Possible to replace "${PYDISTUTILS_INSTALLARGS}/${PORTNAME}${LLVM_SUFFIX}" with "--prefix=${LLVM_PREFIX}".
I didn't find how to disable build of *.py files (*.pyc, *.pyo in pkg-plist) for lit installation (if this matters).
Comment 12 lightside 2016-09-03 16:45:45 UTC
(In reply to comment #11)
> I didn't find how to disable build of *.py files (*.pyc, *.pyo in pkg-plist)
> for lit installation (if this matters).
The `python setup.py --help install` says about --no-compile and --skip-build options, which possible to use to disable this, but I didn't try.
Comment 13 lightside 2016-09-03 17:00:20 UTC
Created attachment 174348 [details]
Proposed patch (since 421246 revision, third variant)

(In reply to comment #12)
> The `python setup.py --help install` says about --no-compile and
> --skip-build options, which possible to use to disable this, but I didn't try.
I tried this. The "--no-compile" option was enough. Attached this variant instead.
Comment 14 lightside 2016-09-30 23:01:42 UTC
Created attachment 175323 [details]
Proposed patch (since 421246 revision, third variant)

Cosmetic fixes.

Clarified description for overall changes:
- Bump PORTREVISION
- Add license information
- Use VAR option helper for _USES_PYTHON variable
- Simplify PORTDOCS with * value
- Add UTILS (default) option to install utility binaries (e.g. count, FileCheck, not, etc.)
- Change description for LIT option
- Add dependency on UTILS for LIT option
- Use new GH_SUBDIR option helper, instead of post-extract-* targets
- Add libxml2 to USE_GNOME for CLANG option, because c-index-test requires this, according to stage-qa
- Sort variables for options (e.g. GOLD_*)
- Remove second OPTIONS_SUB definition
- Replace blank space(s) with tab for some cases
- Build compiler-rt as a project in projects/compiler-rt directory
- Add dependency on CLANG for COMPILER_RT option
- Adapt variables for COMPILER_RT option
- Move installation of llvm-wrapper.sh to post-install-script target
- Remove macho-dump from COMMANDS and clang-modernize from EXTRAS_COMMANDS, which checked with using check-commands target
- Remove regression-test target; use new TEST_TARGET and TEST_WRKSRC variables for `make test`
- Remove post-patch target for lit changes, which allows usage of testing targets
- Install lit with using utils/lit/setup.py; add Python setuptools to build dependency for LIT option
- Adapt build-plist target
- Remove curl and jq requirement with using available commands from base installation of FreeBSD operating system in files/gen-Makefile.snapshot.sh script
- Add files/llvm-lit-wrapper.sh.in, which contains PYTHONPATH environment variable, for correct (llvm-)lit${LLVM_SUFFIX} installation and usage
- Adapt pkg-plist
Comment 15 lightside 2016-10-07 11:49:54 UTC
Created attachment 175490 [details]
Proposed patch (since 423371 revision)

Updated patch, after ports r423371 changes.

- Bump PORTREVISION
- Add license information
- Simplify PORTDOCS with * value
- Add UTILS (default) option to install utility binaries (e.g. count, FileCheck, not, etc.)
- Add remaining parts for files/clang-patch-fopenmp.diff, based on ports r421280 changes
- Change description for LIT option
- Add dependency on UTILS for LIT option
- Sort variables for options (e.g. GOLD_*)
- Remove second OPTIONS_SUB definition
- Replace blank space(s) with tab for some cases
- Build compiler-rt as a project in projects/compiler-rt directory
- Adapt variables for COMPILER_RT option
- Move installation of llvm-wrapper.sh to post-install-script target
- Remove macho-dump from COMMANDS and clang-modernize from EXTRAS_COMMANDS, which checked with using check-commands target
- Remove regression-test target; use new TEST_TARGET and TEST_WRKSRC variables for `make test`
- Remove post-patch target for lit changes, which allows usage of testing targets
- Install lit with using utils/lit/setup.py; add Python setuptools to build dependency for LIT option
- Adapt build-plist target
- Remove curl and jq requirement with using available commands from base installation of FreeBSD operating system in files/gen-Makefile.snapshot.sh script
- Add files/llvm-lit-wrapper.sh.in, which contains PYTHONPATH environment variable, for correct (llvm-)lit${LLVM_SUFFIX} installation and usage
- Adapt pkg-plist
Comment 16 lightside 2016-10-07 11:59:36 UTC
Created attachment 175491 [details]
Proposed patch (since 423371 revision)

Cosmetic fix.
Comment 17 lightside 2016-10-07 13:00:31 UTC
Created attachment 175493 [details]
Proposed patch (since 423371 revision)

Some additions for build-plist target, related to py${PYTHON_VER}.
Comment 18 lightside 2016-10-07 13:13:11 UTC
Created attachment 175494 [details]
Proposed patch (since 423371 revision)
Comment 19 lightside 2016-10-25 04:42:00 UTC
Created attachment 176130 [details]
Proposed patch (since 424411 revision)

Updated patch, after ports r424411 changes.
Comment 20 lightside 2016-10-25 04:46:43 UTC
(In reply to comment #19)
> Updated patch, after ports r424411 changes.
Cosmetic changes to unchanged parts. Previous patch also may apply.
Comment 21 lightside 2016-11-16 13:00:51 UTC
Created attachment 177072 [details]
Proposed patch (since 426205 revision)

Updated patch, after ports r426205 changes.

Removed applied changes:
- Add remaining parts for files/clang-patch-fopenmp.diff, based on ports r421280 changes

Added changes:
- Add llvm-xray to COMMANDS
- Spell CHOSEN_COMPILER_TYPE correctly (ports/199098)
- Convert clang-patch-fopenmp.diff and patch-tools_llvm-shlib_CMakeLists.txt patches in files directory to UTC date format

Overall remaining proposed changes:
- Bump PORTREVISION
- Add license information
- Simplify PORTDOCS with * value
- Add UTILS (default) option to install utility binaries (e.g. count, FileCheck, not, etc.)
- Change description for LIT option
- Add dependency on UTILS for LIT option
- Sort variables for options (e.g. GOLD_*)
- Remove second OPTIONS_SUB definition
- Replace blank space(s) with tab for some cases
- Build compiler-rt as a project in projects/compiler-rt directory
- Adapt variables for COMPILER_RT option
- Spell CHOSEN_COMPILER_TYPE correctly (ports/199098)
- Move installation of llvm-wrapper.sh to post-install-script target
- Remove macho-dump from COMMANDS and clang-modernize from EXTRAS_COMMANDS, which checked with using check-commands target
- Add llvm-xray to COMMANDS
- Remove regression-test target; use new TEST_TARGET and TEST_WRKSRC variables for `make test`
- Remove post-patch target for lit changes, which allows usage of testing targets
- Install lit with using utils/lit/setup.py; add Python setuptools to build dependency for LIT option
- Adapt build-plist target
- Remove curl and jq requirement with using available commands from base installation of FreeBSD operating system in files/gen-Makefile.snapshot.sh script
- Add files/llvm-lit-wrapper.sh.in, which contains PYTHONPATH environment variable, for correct (llvm-)lit${LLVM_SUFFIX} installation and usage
- Convert clang-patch-fopenmp.diff and patch-tools_llvm-shlib_CMakeLists.txt patches in files directory to UTC date format
- Adapt pkg-plist
Comment 22 lightside 2016-11-16 13:06:15 UTC
Created attachment 177073 [details]
Proposed patch (since 426205 revision)
Comment 23 lightside 2016-11-16 13:12:34 UTC
To Brooks Davis:
What issues do you have with remaining proposed changes in this PR?
What are the reasons to not include them (even after converting to your pkg-plist approach)?

If remaining proposed changes are unwanted, then better just to close this PR, in my opinion. There were unnecessary extra work, related to support of this PR, because of other (external) changes to the port.
Comment 24 lightside 2016-11-16 13:37:14 UTC
Created attachment 177076 [details]
The devel/llvm-devel port (v4.0.d20161115) with proposed changes

Added complete devel/llvm-devel port with proposed changes in tar.bz2 archive, just in case.
Comment 25 lightside 2016-11-16 13:46:20 UTC
Current issues (since 426205 revision), found by portlint program (from ports-mgmt/portlint port):
-8<--
% cd /usr/ports/devel/llvm-devel && portlint -C
FATAL: Makefile: [47]: use a tab (not space) after a variable name
FATAL: Makefile: [140]: use a tab (not space) after a variable name
FATAL: Makefile: [230]: use a tab (not space) after a variable name
WARN: Makefile: [255]: use a tab (not space) after a variable name
WARN: Makefile: [43]: You may remove pkg-plist if you use PLIST_FILES and/or PLIST_DIRS.
WARN: Makefile: [0]: possible direct use of command "awk" found. use ${AWK} instead.
WARN: Makefile: possible use of absolute pathname "/omp.h|${EXTRAS_PATT...".
WARN: Makefile: Consider defining LICENSE.
WARN: /usr/ports/devel/llvm-devel/files/patch-tools_llvm-shlib_CMakeLists.txt: patch was not generated using ``make makepatch''.  It is recommended to use ``make makepatch'' when you need to [re-]generate a patch to ensure proper patch format.
3 fatal errors and 6 warnings found.
-->8-

After applied patch in attachment #177076 [details].
-8<--
% portlint -C
WARN: Makefile: [54]: You may remove pkg-plist if you use PLIST_FILES and/or PLIST_DIRS.
WARN: Makefile: possible use of absolute pathname "/lib/clang/${LLVM_RE...".
WARN: Makefile: possible use of absolute pathname "/omp.h|${EXTRAS_PATT...".
0 fatal errors and 3 warnings found.
-->8-
Comment 26 lightside 2016-11-16 13:49:01 UTC
(In reply to comment #25)
> After applied patch in attachment #177076 [details].
After applied patch in attachment #177073 [details].
Comment 27 lightside 2016-11-16 14:29:35 UTC
Example of output for LLVM regression tests (TEST_TARGET=check):
-8<--
% make test
...
Testing Time: 531.50s
  Expected Passes    : 18090
  Expected Failures  : 119
  Unsupported Tests  : 465
-->8-

Example of output for Clang regression tests:
-8<--
% make TEST_TARGET=check-clang test
...
********************
Testing Time: 402.42s
********************
Failing Tests (302):
...
  Expected Passes    : 9698
  Expected Failures  : 16
  Unsupported Tests  : 46
  Unexpected Failures: 302
...
-->8-

Example of output for Clang extra tools' regression tests:
-8<--
% make TEST_TARGET=check-clang-tools test
...
Testing Time: 19.52s
  Expected Passes    : 446
  Expected Failures  : 1
-->8-

Results for v4.0.d20161115 on FreeBSD 10.2 amd64.
Comment 28 lightside 2017-01-16 03:23:45 UTC
Some proposed changes were committed. This PR closed.