Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

quic: update ddd makefile and demo sources #22542

Closed
wants to merge 1 commit into from

Conversation

jamuir
Copy link
Member

@jamuir jamuir commented Oct 27, 2023

Update makefile and fix some signedness issues in the demo sources. Also, determine the length of the message we are sending and send that many bytes (rather than send sizeof the buffer storing the message).

These changes are part of openssl/project#253

doc/designs/ddd/Makefile Outdated Show resolved Hide resolved
doc/designs/ddd/Makefile Show resolved Hide resolved
ddd-06-mem-uv-quic: ddd-06-mem-uv.c
$(CC) $(CFLAGS) $(LDFLAGS) -DUSE_QUIC -o "$@" "$<" $(LDLIBS) -luv
ddd-06-mem-uv-quic: LDLIBS = $(LDLIBS_BASE) -luv
ddd-06-mem-uv-tls: LDLIBS = $(LDLIBS_BASE) -luv
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These two don't look right. Are they a GNUMake-ism?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In GNU make, this is called "target specific variable values":

https://www.gnu.org/software/make/manual/make.html#Target_002dspecific

I am not certain how well it is supported by other versions of make. It may very well be a GNU make-ism.

This makefile uses pattern rules (i.e. rules that use %), so it was already broken for make versions that do not have that (like nmake).

The thing I wanted to make more clear in the makefile was that all the demos are built the same way, except that demo ddd-06 needs to link against libuv.

We could just use the same LDLIBS options for building each ddd demo, and that would simpler.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should avoid GNU specific stuff IMO.

Pattern rules date way way back, I'm less concerned about them. Other Makefiles in the demos area use them, so redressing this should be a separate PR IMO.

Copy link
Member Author

@jamuir jamuir Oct 29, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

understood.

The question remains whether or not target specific variable values is GNU make specific. I do not have enough experience with other make implementations to judge that.

I found that Ubuntu has a NetBSD make package. I installed it, but I cannot get "bmake" to build any of these demos. It does not seem to support pattern rules.

Copy link
Member Author

@jamuir jamuir Oct 31, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've just pushed an update. Let me know if you are okay with this version of the makefile.

doc/designs/ddd/ddd-01-conn-blocking.c Outdated Show resolved Hide resolved
doc/designs/ddd/ddd-01-conn-blocking.c Show resolved Hide resolved
doc/designs/ddd/ddd-02-conn-nonblocking-threads.c Outdated Show resolved Hide resolved
doc/designs/ddd/ddd-03-fd-blocking.c Show resolved Hide resolved
doc/designs/ddd/ddd-04-fd-nonblocking.c Outdated Show resolved Hide resolved
doc/designs/ddd/ddd-05-mem-nonblocking.c Outdated Show resolved Hide resolved
doc/designs/ddd/ddd-06-mem-uv.c Outdated Show resolved Hide resolved
doc/designs/ddd/ddd-06-mem-uv.c Show resolved Hide resolved
@jamuir
Copy link
Member Author

jamuir commented Oct 29, 2023

Thanks for your comments, Pauli.

I've fixed all the indentation issues and will mark those as resolved.

@t8m t8m added branch: master Merge to master branch approval: review pending This pull request needs review by a committer approval: otc review pending This pull request needs review by an OTC member triaged: documentation The issue/pr deals with documentation (errors) tests: exempted The PR is exempt from requirements for testing branch: 3.2 Merge to openssl-3.2 labels Oct 30, 2023
Update makefile and fix some signedness issues in the demo sources.
Drop stray "\n" in the host-port format string that prevented ddd-01
from working (this was also noticed by Neil H). Also, determine the
length of the message we are sending and send that many bytes (rather
than send sizeof the buffer storing the message).

These changes are part of openssl/project#253
@t8m t8m removed the approval: otc review pending This pull request needs review by an OTC member label Oct 31, 2023
@hlandau hlandau added approval: done This pull request has the required number of approvals and removed approval: review pending This pull request needs review by a committer labels Oct 31, 2023
@openssl-machine openssl-machine added approval: ready to merge The 24 hour grace period has passed, ready to merge and removed approval: done This pull request has the required number of approvals labels Nov 1, 2023
@openssl-machine
Copy link
Collaborator

This pull request is ready to merge

@hlandau
Copy link
Member

hlandau commented Nov 1, 2023

Merged to master and 3.2. Thank you.

@hlandau hlandau closed this Nov 1, 2023
openssl-machine pushed a commit that referenced this pull request Nov 1, 2023
Update makefile and fix some signedness issues in the demo sources.
Drop stray "\n" in the host-port format string that prevented ddd-01
from working (this was also noticed by Neil H). Also, determine the
length of the message we are sending and send that many bytes (rather
than send sizeof the buffer storing the message).

These changes are part of openssl/project#253

Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Hugo Landau <hlandau@openssl.org>
Reviewed-by: Paul Dale <pauli@openssl.org>
(Merged from #22542)
openssl-machine pushed a commit that referenced this pull request Nov 1, 2023
Update makefile and fix some signedness issues in the demo sources.
Drop stray "\n" in the host-port format string that prevented ddd-01
from working (this was also noticed by Neil H). Also, determine the
length of the message we are sending and send that many bytes (rather
than send sizeof the buffer storing the message).

These changes are part of openssl/project#253

Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Hugo Landau <hlandau@openssl.org>
Reviewed-by: Paul Dale <pauli@openssl.org>
(Merged from #22542)

(cherry picked from commit d1338fc)
wanghao75 pushed a commit to openeuler-mirror/openssl that referenced this pull request Nov 4, 2023
Update makefile and fix some signedness issues in the demo sources.
Drop stray "\n" in the host-port format string that prevented ddd-01
from working (this was also noticed by Neil H). Also, determine the
length of the message we are sending and send that many bytes (rather
than send sizeof the buffer storing the message).

These changes are part of openssl/project#253

Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Hugo Landau <hlandau@openssl.org>
Reviewed-by: Paul Dale <pauli@openssl.org>
(Merged from openssl/openssl#22542)

Signed-off-by: fly2x <fly2x@hitls.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approval: ready to merge The 24 hour grace period has passed, ready to merge branch: master Merge to master branch branch: 3.2 Merge to openssl-3.2 tests: exempted The PR is exempt from requirements for testing triaged: documentation The issue/pr deals with documentation (errors)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants