DAOS-12710 build: Improve daos/tse.h handling. #13613

ashleypittman · 2024-01-16T14:15:55Z

Fix doxygen comments in daos_task.h and daos/tse.h

Change includes so all public daos headers are independent and can
be included without including any headers prior.
Add a unit test about the above for all installed headers.
Remove the scons check for gurt and cart headers in favour
of the new unit test.

Signed-off-by: Ashley Pittman [email protected]

Remove the dependency between daos_task.h and daos/tse.h The general rule is daos_*.h are public headers and daos/*.h are private headers although daos/tse.h was an exception and installed as part of the RPM. Move the typdes from tse.h to daos_task.h where they're needed and remove some includes from around the tree where tse.h was include but didn't need to be. Fix some doxygen style comments in daos_task.h to be correct doxygen. Required-githooks: true Signed-off-by: Ashley Pittman <[email protected]>

github-actions · 2024-01-16T14:16:29Z

Bug-tracker data:
Ticket title is 'public header includes private header.'
Status is 'In Review'
Labels: 'triaged'
https://daosio.atlassian.net/browse/DAOS-12710

ashleypittman · 2024-01-16T16:50:24Z

@mchaarawi this is technically an API change but did we intend to export daos/tse.h? If there are functions there we want exported then we should move them to daos_task.h IMO.

mchaarawi · 2024-01-16T16:59:47Z

@mchaarawi this is technically an API change but did we intend to export daos/tse.h? If there are functions there we want exported then we should move them to daos_task.h IMO.

not really. daos_task is something different than tse. tse by itself is unrelated to DAOS and meant as a separate package. some libraries use tse without daos. but daos_task is different and depends on DAOS.

ashleypittman · 2024-01-16T17:04:55Z

@mchaarawi this is technically an API change but did we intend to export daos/tse.h? If there are functions there we want exported then we should move them to daos_task.h IMO.

not really. daos_task is something different than tse. tse by itself is unrelated to DAOS and meant as a separate package. some libraries use tse without daos. but daos_task is different and depends on DAOS.

Does anything outside the daos tree use tse directly?

With this PR you can't include daos/tse.h without previously including daos_task.h which goes against what you're saying but I suspect that was the case anyway - I can check.

mchaarawi

tse is used by HDF5 and possible other packages, so this cannot land.

mchaarawi · 2024-01-16T17:58:49Z

@mchaarawi this is technically an API change but did we intend to export daos/tse.h? If there are functions there we want exported then we should move them to daos_task.h IMO.

not really. daos_task is something different than tse. tse by itself is unrelated to DAOS and meant as a separate package. some libraries use tse without daos. but daos_task is different and depends on DAOS.

Does anything outside the daos tree use tse directly?

With this PR you can't include daos/tse.h without previously including daos_task.h which goes against what you're saying but I suspect that was the case anyway - I can check.

daos_task.h depends on tse.h not the other way around. so this PR is not correct.

Add daos/tse.h to doxygen file and fix annotations. Required-githooks: true Signed-off-by: Ashley Pittman <[email protected]>

Required-githooks: true Signed-off-by: Ashley Pittman <[email protected]>

ashleypittman · 2024-01-16T18:36:26Z

tse is used by HDF5 and possible other packages, so this cannot land.

Ok, updated on that basis. daos/tse.h was and still is includable without pulling in any daos_*.h headers.

mchaarawi · 2024-01-16T23:15:29Z

src/common/tse.c

@@ -17,8 +17,10 @@

 #include <stdint.h>
 #include <pthread.h>
+#include <daos_task.h>


where does this file require daos_task.h ?

mchaarawi · 2024-01-16T23:16:34Z

src/include/daos/common.h

 #include <daos_types.h>
 #include <daos_obj.h>
 #include <daos_prop.h>
 #include <daos_security.h>
+#include <daos_task.h>


why add daos_task.h here?

mchaarawi · 2024-01-16T23:16:52Z

src/include/daos/common.h

 #include <daos/profile.h>
 #include <daos/dtx.h>
 #include <daos/cmd_parser.h>
+#include <daos/tse.h>


same, why is that needed here?

src/include/daos/dpar.h

mchaarawi · 2024-01-16T23:18:59Z

src/include/daos/tse.h

 typedef struct tse_task {
+	/** the result */


that sounds like a useless comment.

"result of the task operation - valid after task completion"

mchaarawi · 2024-01-16T23:19:16Z

src/include/daos/tse.h

 #define TSE_TASK_SIZE		(TSE_PRIV_SIZE + 8)

+/** A single task */


that sounds like a useless comment.

Our documentation requires all structures and entries to be documented, some text is needed here, feel free to suggest something better.

"struct for a task object that is used for tracking an operation"

mchaarawi · 2024-01-16T23:24:44Z

src/include/daos_api.h

+#include <daos_types.h>
+


so even though those headers are installed, i believe we discussed at one point that really the user should just include daos.h for the daos API, then add daos_fs.h, or others for the higher level APIs vs being able to add each header. so your changes to make each public header byitself pull the other needed ones is not necessary.

Required-githooks: true

Required-githooks: true Signed-off-by: Ashley Pittman <[email protected]>

partially working. Required-githooks: true Signed-off-by: Ashley Pittman <[email protected]>

daosbuild1 · 2024-01-18T09:19:05Z

Test stage Functional Hardware Medium completed with status UNSTABLE. https://build.hpdd.intel.com/job/daos-stack/job/daos//view/change-requests/job/PR-13613/6/testReport/

Required-githooks: true Signed-off-by: Ashley Pittman <[email protected]>

daltonbohning · 2024-01-18T19:23:07Z

src/include/daos/mgmt.h

-#include <daos/common.h>
-#include <daos/tse.h>


This seems improper, or at least not how I'm familiar with imports. Generally, if a file needs a definition, it should import the header that contains that definition.
For example, mgmt.h uses tse_task_t, which is defined in tse.h. So mgmt.h should include tse.h directly instead of implicitly relying on other imports to import those definitions, which could change.

You're correct. I started this PR assuming that tse.h was a private header and trying to remove it's special handing but as @mchaarawi pointed it it's part of the API and it's special handing is warranted. An early change I made incorrectly meant that daos_task.h neeed to included before daos/tse.h and these changes were to support that.

What you're saying about includes only being included to satisfy the header they're included in is good practice and this PR is an attempt to improve on that for the public headers but we do tend to have a few header files that just catch everything and I didn't mean to exacerbate that.

I've backed those changes out now and this PR is smaller as a result, thank you.

The other issue here is that we should also be using <> for public headers and "" for internal ones, plus including system headers first, then daos public, then daos private but I don't think any of our code does that.

We've also got a few headers which are included both with or without a directory in places, daos_test.h is included as "<daos_test>" 25 times, <daos_test.h> 6 times and "<suite/daos_test.h>" once.

daltonbohning · 2024-01-18T19:25:15Z

src/include/daos/common.h

 #include <daos_types.h>
 #include <daos_obj.h>
 #include <daos_prop.h>
 #include <daos_security.h>
+#include <daos/tse.h>


daos/common.h includes daos/tse.h, but daos/common.h doesn't actually need the definitions in daos/tse.h. So this seems improper to me

Required-githooks: true Signed-off-by: Ashley Pittman <[email protected]>

daltonbohning

Please do correct me if I'm wrong about how includes should be organized

daltonbohning · 2024-01-18T20:11:51Z

src/include/daos/container.h

@@ -1,5 +1,5 @@
 /**
- * (C) Copyright 2016-2023 Intel Corporation.
+ * (C) Copyright 2016-2024 Intel Corporation.


erroneous copyright update after changes were backed out. Here and others

I'll do another pass and fix these when I'm done with other changes.

daltonbohning · 2024-01-18T20:13:24Z

src/include/daos/pool.h

-#include <daos/tse.h>
-#include <daos_types.h>
-#include <daos_pool.h>


This file should still include <daos/tse.h> right?

On what basis? That in uses some of the defines in daos/tse.h I assume? We have to assume that every file that includes this already includes daos/tse.h via some route.

It's easy enough to run my new unit test on the private headers as well, I'll update this PR with a few more code changes to allow all of them to be included but I won't enable this by default as it means the test would need to access abt.h and also we have daos/qat.h which we don't seem to include from anywhere today.

I've also found that using the macros from gurt/debug.h depends on gurt/common.h but you can't include gurt/common.h before gurt/debug.h so you have to include gurt/debug.h then gurt/common.h or just gurt/common.h

On what basis? That in uses some of the defines in daos/tse.h I assume?

Right.

We have to assume that every file that includes this already includes daos/tse.h via some route.

That works accidentally, but not in the general sense. daos/pool.h now has a silent dependency on daos/tse.h.

the macros from gurt/debug.h depends on gurt/common.h

That issue is exactly the kind of issue I'm trying to avoid. If "the macros from gurt/debug.h depends on gurt/common.h" then gurt/debug.h should include gurt/common.h

On what basis? That in uses some of the defines in daos/tse.h I assume?

Right.

We have to assume that every file that includes this already includes daos/tse.h via some route.

That works accidentally, but not in the general sense. daos/pool.h now has a silent dependency on daos/tse.h.

Having testing more now I don't think it does, daos/pool.h obviously pulls in daos/tse.h from one of the header files that it includes. We could no doubt do some pruning of includes from c files but getting the headers right is the first step towards that.

the macros from gurt/debug.h depends on gurt/common.h

That issue is exactly the kind of issue I'm trying to avoid. If "the macros from gurt/debug.h depends on gurt/common.h" then gurt/debug.h should include gurt/common.h

It can't, gurt/common.h includes gurt/debug.h. I'll deal with that another day.

This is what I mean. Currently, pool.h has an implicit dependency on tse.h. If, for whatever reason, something.h gets refactored and doesn't need tse.h anymore, then pool.h will be missing an include. And since pool.h already includes tse.h, I don't see the value in removing it.

We could no doubt do some pruning of includes from c files

I don't think the goal should be to reduce the includes to the minimal possible that allows us to build each header independently. We should include what we need in the headers that need it. Otherwise, we end up breaking unrelated files if we reorganize, like in my example above.

daltonbohning · 2024-01-18T20:14:40Z

src/include/daos/task.h

 #include <daos_types.h>
 #include <daos_api.h>
-#include <daos/tse.h>


And this one?

daltonbohning · 2024-01-18T20:16:37Z

src/object/obj_internal.h

@@ -14,7 +14,6 @@
 #include <stdint.h>
 #include <daos/common.h>
 #include <daos/event.h>
-#include <daos/tse.h>


Required-githooks: true Signed-off-by: Ashley Pittman <[email protected]>

daosbuild1 · 2024-01-19T21:41:28Z

Test stage Functional Hardware Medium completed with status UNSTABLE. https://build.hpdd.intel.com/job/daos-stack/job/daos//view/change-requests/job/PR-13613/9/testReport/

Required-githooks: true

Required-githooks: true Signed-off-by: Ashley Pittman <[email protected]>

src/include/daos/common.h

src/include/daos/mgmt.h

mchaarawi · 2024-01-23T21:07:57Z

src/include/daos/sys_debug.h

-#include <daos/common.h>
-#include <daos/tse.h>
-#include <daos_types.h>
+#include <daos_task.h>


so why here add daos_task.h but other places like https://github.com/daos-stack/daos/pull/13613/files#diff-930d69e66aac2e78ef8537ea29d976fc69620cbdeee43d117662b9e6f7f1f036R14 you added aos/tse.h

seems inconsistent.

mchaarawi · 2024-01-23T21:11:28Z

src/include/daos/tse.h

 #define TSE_TASK_SIZE		(TSE_PRIV_SIZE + 8)

+/** A single task */


"struct for a task object that is used for tracking an operation"

mchaarawi · 2024-01-23T21:13:23Z

src/include/daos/tse.h

 typedef struct tse_task {
+	/** the result */


"result of the task operation - valid after task completion"

mchaarawi · 2024-01-23T21:13:53Z

src/include/daos/tse.h

@@ -36,37 +42,41 @@ typedef struct tse_task {
 * Track all of the tasks under a scheduler.
 */
 typedef struct {
+	/** the result */


same as above, but this is for the schedule

mchaarawi · 2024-01-23T21:15:07Z

src/include/daos/tse.h

 typedef int (*tse_sched_comp_cb_t)(void *args, int rc);
+/** Callaback function for when a task is done */


this comment is wrong.

mchaarawi · 2024-01-23T21:18:40Z

src/include/daos_array.h

+#include <daos_types.h>
+#include <daos_obj.h>
+#include <daos_obj_class.h>
+


im not going to comment again on every one of those public headers and will just comment on his one.
so the idea is for user to include daos.h, and that pulls in all what's needed. so "fixing" all the headers to be able to be included by itself is not required.

mchaarawi · 2024-01-23T21:19:14Z

src/include/daos_fs.h

+#include <daos_types.h>
+#include <daos_obj.h>
+#include <daos_obj_class.h>
+#include <daos_array.h>
+#include <daos_cont.h>
+


not required. expectation is that user pulls in daos.h then daos_fs.h

Required-githooks: true Signed-off-by: Ashley Pittman <[email protected]>

mchaarawi · 2024-02-14T19:26:01Z

src/include/daos/tse.h

 typedef int (*tse_sched_comp_cb_t)(void *args, int rc);
+/** A task callback function */


this is not a callback. this is the prototype def of the task body function.

daltonbohning · 2024-02-15T16:27:49Z

utils/utest.yaml

+- name: include checks
+  tests:
+    - cmd: ["utils/include_test.py"]


This is another case where my might want a directory of pytest tests somewhere

ashleypittman marked this pull request as ready for review January 16, 2024 16:49

ashleypittman requested review from a team as code owners January 16, 2024 16:49

ashleypittman requested review from daltonbohning and jolivier23 January 16, 2024 16:50

mchaarawi requested changes Jan 16, 2024

View reviewed changes

ashleypittman added 3 commits January 16, 2024 18:19

Add daos/tse.h back into install list and revert daos_task.h changes.

65ed3be

Add daos/tse.h to doxygen file and fix annotations. Required-githooks: true Signed-off-by: Ashley Pittman <[email protected]>

Add unit test and fix issues.

01013f2

Required-githooks: true Signed-off-by: Ashley Pittman <[email protected]>

Make daos/tse.h includable.

0d82757

Required-githooks: true Signed-off-by: Ashley Pittman <[email protected]>

ashleypittman requested a review from a team as a code owner January 16, 2024 18:28

Add the new test script.

a4899cf

Required-githooks: true Signed-off-by: Ashley Pittman <[email protected]>

ashleypittman changed the title ~~DAOS-12710 build: Do not install daos/tse.h.~~ DAOS-12710 build: Improve daos/tse.h handling. Jan 16, 2024

Fix spelling.

39596a0

Required-githooks: true Signed-off-by: Ashley Pittman <[email protected]>

ashleypittman requested a review from mchaarawi January 16, 2024 19:38

mchaarawi requested changes Jan 16, 2024

View reviewed changes

ashleypittman added 3 commits January 17, 2024 15:51

Merge branch 'master' into amd/tse-include

38f400d

Required-githooks: true

Remove some inlcudes.

bcfc626

Required-githooks: true Signed-off-by: Ashley Pittman <[email protected]>

Manaully update copyrights, the commit hook seems to be only

60a0779

partially working. Required-githooks: true Signed-off-by: Ashley Pittman <[email protected]>

ashleypittman mentioned this pull request Jan 18, 2024

DAOS-12710 build: Add unit testing that header can be included. #13617

Closed

Take on feedback on the test.

5aadc0e

Required-githooks: true Signed-off-by: Ashley Pittman <[email protected]>

daltonbohning reviewed Jan 18, 2024

View reviewed changes

Back out some tse changes.

745da2d

Required-githooks: true Signed-off-by: Ashley Pittman <[email protected]>

daltonbohning reviewed Jan 18, 2024

View reviewed changes

Make all private headers directly includable as well.

5427124

Required-githooks: true Signed-off-by: Ashley Pittman <[email protected]>

ashleypittman added 2 commits January 22, 2024 11:39

Merge branch 'master' into amd/tse-include

cf669f0

Required-githooks: true

Add back in some ioncludes and revert date changes where possible.

9f41b40

Required-githooks: true Signed-off-by: Ashley Pittman <[email protected]>

ashleypittman requested a review from mchaarawi January 22, 2024 19:46

mchaarawi requested changes Jan 23, 2024

View reviewed changes

ashleypittman added 2 commits January 29, 2024 10:31

Take on review feedback.

0f7d53a

Required-githooks: true Signed-off-by: Ashley Pittman <[email protected]>

Fix spelling.

ae7e5f0

Required-githooks: true Signed-off-by: Ashley Pittman <[email protected]>

ashleypittman requested a review from mchaarawi February 13, 2024 11:59

mchaarawi approved these changes Feb 15, 2024

View reviewed changes

ashleypittman requested review from a team, soumagne and daltonbohning February 15, 2024 14:26

daltonbohning reviewed Feb 15, 2024

View reviewed changes

daltonbohning approved these changes Feb 15, 2024

View reviewed changes

ashleypittman requested a review from a team February 15, 2024 17:11

ashleypittman merged commit a559d12 into master Feb 15, 2024
49 checks passed

ashleypittman deleted the amd/tse-include branch February 15, 2024 17:22

		#define TSE_TASK_SIZE (TSE_PRIV_SIZE + 8)

		/** A single task */

		typedef int (tse_sched_comp_cb_t)(void args, int rc);
		/** Callaback function for when a task is done */

		typedef int (tse_sched_comp_cb_t)(void args, int rc);
		/** A task callback function */

DAOS-12710 build: Improve daos/tse.h handling. #13613

DAOS-12710 build: Improve daos/tse.h handling. #13613

Conversation

ashleypittman commented Jan 16, 2024 • edited Loading

github-actions bot commented Jan 16, 2024

ashleypittman commented Jan 16, 2024

mchaarawi commented Jan 16, 2024

ashleypittman commented Jan 16, 2024

mchaarawi left a comment

Choose a reason for hiding this comment

mchaarawi commented Jan 16, 2024

ashleypittman commented Jan 16, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daosbuild1 commented Jan 18, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daltonbohning left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daosbuild1 commented Jan 19, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ashleypittman commented Jan 16, 2024 •

edited

Loading