Bug 231770 - databases/sqlite3: Add ICU flavor
Summary: databases/sqlite3: Add ICU flavor
Status: Closed FIXED
Alias: None
Product: Ports & Packages
Classification: Unclassified
Component: Individual Port(s) (show other bugs)
Version: Latest
Hardware: Any Any
: --- Affects Only Me
Assignee: Dmitry Marakasov
URL:
Keywords:
Depends on: 232248
Blocks:
  Show dependency treegraph
 
Reported: 2018-09-27 21:10 UTC by fsbruva
Modified: 2021-03-15 17:25 UTC (History)
1 user (show)

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


Attachments
Add flavors - default and icu (1.67 KB, patch)
2018-09-27 21:10 UTC, fsbruva
no flags Details | Diff
Flavor: default, icu & mini (1.42 KB, patch)
2018-10-02 07:20 UTC, Pavel Volkov
no flags Details | Diff
Add flavors - default, icu and mini (16.26 KB, patch)
2018-10-03 04:31 UTC, fsbruva
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description fsbruva 2018-09-27 21:10:34 UTC
Created attachment 197558 [details]
Add flavors - default and icu

I have a need to use p5-DBD-SQLite with ICU support. Adding the ICU flavor to this port will make this possible with the latest and most secure sqlite3 available.
Comment 1 Pavel Volkov 2018-10-02 07:20:09 UTC
Created attachment 197702 [details]
Flavor: default, icu & mini

Hello Matt.
I liked the use of the flavor.
Maybe you approve of my patch? Take a look at it. Using the default options implies the absence of a custom port configuration.
Comment 2 fsbruva 2018-10-02 16:11:12 UTC
See: https://www.freebsd.org/doc/en/books/porters-handbook/flavors-using.html. Look especially example 7.3, where flavors directly control build variables, rather than port OPTIONS.

I looked at your patch, but cannot test until tonight. I believe using the OPTIONS_DEFAULT will permit the proper package creation install via 'pkg'. However, I believe this method can be broken by the user if built from the ports tree. This is because regardless of the OPTIONS_DEFAULT value(s), the user can de-select the ICU option during 'make config'. In this case, the resulting package will have the -icu suffix once installed, but SQLite won't actually be linked against ICU. This will lead to unacceptable ambiguity. 

This is why my patch completely overrides the OPTIONS list, and makes the ICU dependency non-optional for the -icu flavor. I will test tonight, and revise my patch to include your proposed "mini" flavor.
Comment 3 fsbruva 2018-10-03 04:31:58 UTC
Created attachment 197729 [details]
Add flavors - default, icu and mini

I tested your patch tonight - and it behaved as I expected: users can de-select the "ICU" option during `make config`. This is not appropriate.

I looked more closely at the Makefile for editors/emacs, and emulated the use of OPTIONS_EXCLUDE to force the relevant build behavior for the icu and mini flavors. While attempting that, I completely re-organized the Makefile to conform to the guidance of [1], especially [2]. Following the major edit, I audited the two versions to ensure I hadn't missed anything.

[1] https://www.freebsd.org/doc/en/books/porters-handbook/porting-order.html
[2] https://www.freebsd.org/doc/en/books/porters-handbook/porting-order-options.html
Comment 4 Pavel Volkov 2018-10-14 15:15:57 UTC
Hello. I am sorry.
I can not get used to the list of options sorted by you. I do not see where the description of one option begins, where it ends. And what this option does. I am not ready to accept the application of recommendations listed in the documentation.
I made other suggested fixes to my patch. (https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=232248)
Thanks for the suggestion.
Comment 5 fsbruva 2018-10-14 19:58:28 UTC
(In reply to Pavel Volkov from comment #4)
No problem. I was just trying to follow the style guidance in the FreeBSD handbook. :-)
Comment 6 Dmitry Marakasov freebsd_committer freebsd_triage 2021-03-15 17:25:53 UTC
Assuming this was fixed in https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=232248