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

ntp: resection manual-pages according to IPD4, rebuild for library/se… #19993

Open
wants to merge 1 commit into
base: oi/hipster
Choose a base branch
from

Conversation

klausz65
Copy link
Contributor

…curity/openssl-3

@klausz65 klausz65 marked this pull request as draft December 13, 2024 09:06
@@ -44,6 +45,7 @@ COMPONENT_FMRI= service/network/ntp
COMPONENT_CLASSIFICATION= System/Services
COMPONENT_LICENSE= ntp license
COMPONENT_LICENSE_FILE= COPYRIGHT
COMPONENT_BUILD_ARGS= -j20
Copy link
Contributor

Choose a reason for hiding this comment

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

Please never do this. Use USE_PARALLEL_BUILD instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed

@@ -53,6 +55,7 @@ include $(WS_MAKE_RULES)/common.mk

# We modified configure.ac and m4 macros
COMPONENT_PREP_ACTION = ( cd $(@D) && PATH="$(PATH)" ./bootstrap )
COMPONENT_POST_CONFIGURE_ACTION += ( cd $(BUILD_DIR_64)/ntpd; gmake ntp_parser.h)
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add a comment explaining why is this needed. Thank you.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed

*/
-#ifdef HAVE_SYS_AUDIOIO_H
-static struct audio_device device; /* audio device ident */
-#endif /* HAVE_SYS_AUDIOIO_H */
Copy link
Contributor

Choose a reason for hiding this comment

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

This change is not related to paths customization (see top of this file). If this is really needed please add such a change as a new patch with a comment explaining why it is needed. Thank you.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed

@@ -19,7 +19,7 @@
# CDDL HEADER END
#
# Copyright (c) 2011, 2013, Oracle and/or its affiliates. All rights reserved.
# Copyright (c) 2023, Friedrich Kink. All rights reserved.
Copy link
Contributor

Choose a reason for hiding this comment

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

Please never remove copyrights of others without their consent.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

changed back

Copy link
Contributor

Choose a reason for hiding this comment

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

Where is Friedrich Kink? What exactly you added back?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

have forgotten git add for ntp.p5m

@@ -0,0 +1,11 @@
--- ntpd/Makefile.am.orig 2023-05-31 12:31:38.000000000 +0200
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add a comment explaining why we do need this. Thank you.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed

@@ -0,0 +1,62 @@
--- util/ntptime.c.orig 2020-03-04 00:41:29.000000000 +0100
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add a comment explaining why we do need this. Thank you.

Copy link
Contributor

Choose a reason for hiding this comment

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

BTW, the patch filename contains typo (two r, missing n).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed

@klausz65 klausz65 force-pushed the ntp branch 4 times, most recently from 702979f to a023fb6 Compare December 14, 2024 11:34
@klausz65 klausz65 marked this pull request as ready for review December 14, 2024 11:34
@klausz65
Copy link
Contributor Author

The new results-all.master test file needs pull #20032

@mtelka
Copy link
Contributor

mtelka commented Dec 14, 2024

The new results-all.master test file needs pull #20032

It does not. You can simply do something like COMPONENT_TEST_ARGS += -k here.

@@ -26,12 +26,14 @@
# Copyright (c) 2023, Niklas Poslovski
#

# for the moment the SHAKE128 digest fails
USE_DEFAULT_TEST_TRANSFORMS= yes
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the results-all.master reproducible with this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes

@@ -26,12 +26,14 @@
# Copyright (c) 2023, Niklas Poslovski
#

# for the moment the SHAKE128 digest fails
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe this comment does not belong here. Why do we need it at all? Isn't the test failure visible in results-all.master. If not, why?

Isn't it possible to skip the test instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed

@@ -19,7 +19,7 @@
# CDDL HEADER END
#
# Copyright (c) 2011, 2013, Oracle and/or its affiliates. All rights reserved.
# Copyright (c) 2023, Friedrich Kink. All rights reserved.
Copy link
Contributor

Choose a reason for hiding this comment

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

Where is Friedrich Kink? What exactly you added back?

sntp : tests/test-suite.log
=========================================

FAIL: test-crypto
Copy link
Contributor

Choose a reason for hiding this comment

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

With the new results-all.master I miss the list of tests. It is always good to have such a list so we are able to detect when the number of failing tests didn't change, but instead of test1 failure we now see the test2 failing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't get you here

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't get you here

Since you reverted this change then it makes little sense to discuss it.

@klausz65 klausz65 force-pushed the ntp branch 3 times, most recently from 487a858 to d6788d4 Compare December 14, 2024 14:17
@@ -54,6 +54,8 @@ include $(WS_MAKE_RULES)/common.mk
# We modified configure.ac and m4 macros
COMPONENT_PREP_ACTION = ( cd $(@D) && PATH="$(PATH)" ./bootstrap )

COMPONENT_TEST_ARGS += -k
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add a comment explaining why is this needed. Thank you.

@@ -1,22 +1,25 @@
crypto.c:14:test_MakeSHAKE128Mac:PASS
make_mac: MAC SHAKE128 Digest Final failed.
Copy link
Contributor

Choose a reason for hiding this comment

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

This is apparently a regression. I've no idea what would be the impact of this but it could possibly cause that something won't work as expected. Since ntpd is core part of system (and I would like to avoid reports that people are no longer able to sync their time properly) then please try to find why is this failing now.

Thank you.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I allready tried, but sorry I'm too dump to fix this, if you want to have that IPD4 part, you'll have to take this PR as it is otherwise tell me and I'll cancel it.

Copy link
Contributor

Choose a reason for hiding this comment

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

I allready tried, but sorry I'm too dump to fix this, if you want to have that IPD4 part, you'll have to take this PR as it is otherwise tell me and I'll cancel it.

Quick search revealed that the problem is very likely caused by a change in OpenSSL 3.4.

Related links:

Copy link
Contributor Author

@klausz65 klausz65 Dec 14, 2024

Choose a reason for hiding this comment

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

I can't fix this - sorry

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants