Page MenuHomeFreeBSD

Build options etc for libbearssl and libve
ClosedPublic

Authored by sjg on Jul 19 2018, 12:30 AM.

Diff Detail

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

Event Timeline

sjg created this revision.Jul 19 2018, 12:30 AM
sjg added a reviewer: imp.Jul 19 2018, 12:33 AM
bdrewery requested changes to this revision.Sep 5 2018, 9:44 PM

Can't evaluate lib/Makefile changes without the rest of the changes. I.e., does it require dependency changes in Makefile.inc1's make libraries handling? Can't tell from this.

share/mk/src.libnames.mk
216–223 ↗(On Diff #45507)

These should be something like ${LIBBEARSSLDIR}/libbearssl.a rather than ${DESTDIR}${LIBDIR} since internallibs never get installed.
The FOODIR var does get defaulted to lib/libfoo later on.

This revision now requires changes to proceed.Sep 5 2018, 9:44 PM
sjg added a comment.Sep 5 2018, 10:00 PM

Sorry, should make all these reviews xref each other.
we have

D16336 for changes to stand/
D16335 for libve
D16334 for libbearssl

sjg updated this revision to Diff 47712.Sep 5 2018, 10:12 PM

Fix INTERNALLIB refs

sjg marked an inline comment as done.Sep 5 2018, 10:12 PM
bdrewery accepted this revision.Sep 6 2018, 12:04 AM
This revision is now accepted and ready to land.Sep 6 2018, 12:04 AM
emaste added inline comments.Jan 10 2019, 3:57 PM
lib/Makefile
168–169 ↗(On Diff #47712)

This can use SUBDIR.${MK_BEARSSL}+=libbearssl libve style, no?

tools/build/options/WITH_BEARSSL
2 ↗(On Diff #47712)

Options files typically start with a one-sentence description and I think that improves the flow of the generated man page. (They also typically start with Set to build or Set to not build, which I don't think improves readability.)

E.g., just start with "Build the BearSSL library."

4 ↗(On Diff #47712)

Is there specific markup for URLs?

6–10 ↗(On Diff #47712)

This out-of-tree bearssl src should go.

sjg added inline comments.Jan 10 2019, 6:12 PM
tools/build/options/WITH_BEARSSL
6–10 ↗(On Diff #47712)

The intent was that it would eventually end up in contrib/ but didn't think that should be a gating factor?

emaste added inline comments.Jan 10 2019, 6:21 PM
tools/build/options/WITH_BEARSSL
6–10 ↗(On Diff #47712)

If we're ready to start adding the build goop we should just put it in contrib

imp added inline comments.Jan 10 2019, 8:12 PM
tools/build/options/WITH_BEARSSL
6–10 ↗(On Diff #47712)

We should just import it, even if it isn't in the default built, I agree. Having out-of-tree support for it I think slows the adoption of it for the base.

sjg updated this revision to Diff 52839.Jan 14 2019, 9:56 PM

Update per feedback

This revision now requires review to proceed.Jan 14 2019, 9:56 PM
share/mk/src.libnames.mk
219 ↗(On Diff #52839)

This should probably be

LIBSECUREBOOT?= ${LIBSECUREBOOTDIR}/libsecureboot.a

share/mk/src.opts.mk
224 ↗(On Diff #52839)

Since the verification library is now called "libsecureboot" perhaps we should rename this to "LOADER_SECUREBOOT" or something similar.

Overall looks good to me now, modulo the couple of outstanding comments.

tools/build/options/WITH_BEARSSL
4 ↗(On Diff #47712)

Found it, .Lk

sjg added inline comments.Jan 16 2019, 11:25 PM
share/mk/src.libnames.mk
219 ↗(On Diff #52839)

Thanks

share/mk/src.opts.mk
224 ↗(On Diff #52839)

That depends a bit on your plans.
LOADER_VERIEXEC is a reasonable description of what my patches currently do
LOADER_SECUREBOOT could also be valid but you might want a separate knob to select different behavior - with LUA in loader there is not a lot of headroom.

sjg added inline comments.Jan 16 2019, 11:34 PM
tools/build/options/WITH_BEARSSL
4 ↗(On Diff #47712)

Where?

Is the syntax

.Lk http://www.BearSSL.org/

?

sjg updated this revision to Diff 52903.Jan 16 2019, 11:49 PM

Fix src.libnames.mk

emaste added inline comments.Jan 17 2019, 2:51 AM
tools/build/options/WITH_BEARSSL
4 ↗(On Diff #47712)

from mdoc(7):

Lk
  Format a hyperlink.  Its syntax is as follows:

        .Lk uri [name]

  Examples:
        .Lk http://bsd.lv "The BSD.lv Project"
        .Lk http://bsd.lv

  See also Mt.
emaste accepted this revision.Feb 25 2019, 9:07 PM

LGTM (with a few notes), let's commit and adjust further if necessary

share/mk/src.libnames.mk
219 ↗(On Diff #52839)

and now libsecureboot${PIE_FLAG}.a

tools/build/options/WITH_BEARSSL
7–11 ↗(On Diff #52903)

Yes, let's just build the in-tree one.

tools/build/options/WITH_VERIEXEC
7–9 ↗(On Diff #52903)

If the dependencies are encoded in share/mk/* makeman will automatically add a sentence to this effect (something like, for WITHOUT_BEARSSL, When set, it enforces these options: WITHOUT_VERIEXEC).

In any case I think it'd be fine to commit as is, changing the wording later if it doesn't read well.

This revision is now accepted and ready to land.Feb 25 2019, 9:07 PM
Closed by commit rS344566: Enable build of libbearssl (authored by sjg, committed by ). · Explain WhyFeb 26 2019, 6:11 AM
This revision was automatically updated to reflect the committed changes.