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

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

Closed
wants to merge 5 commits into from

Conversation

ashleypittman
Copy link
Contributor

Add a unit test to verify all installed headers can be included
standalone.

Remove scons check for cart and gurt headers for the same.

Required-githooks: true

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

Add a unit test to verify all installed headers can be included
standalone.

Remove scons check for cart and gurt headers for the same.

Required-githooks: true

Signed-off-by: Ashley Pittman <[email protected]>
@daosbuild1
Copy link
Collaborator

Test stage Python Bandit check completed with status FAILURE. https://build.hpdd.intel.com//job/daos-stack/job/daos/view/change-requests/job/PR-13617/2/execution/node/124/log

Copy link

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

@daosbuild1
Copy link
Collaborator

Test stage Unit Test on EL 8.8 completed with status FAILURE. https://build.hpdd.intel.com//job/daos-stack/job/daos/view/change-requests/job/PR-13617/2/execution/node/733/log

Required-githooks: true
Signed-off-by: Ashley Pittman <[email protected]>
Required-githooks: true
Signed-off-by: Ashley Pittman <[email protected]>
@daosbuild1
Copy link
Collaborator

Test stage Unit Test on EL 8.8 completed with status UNSTABLE. https://build.hpdd.intel.com/job/daos-stack/job/daos//view/change-requests/job/PR-13617/3/testReport/

@daosbuild1
Copy link
Collaborator

Test stage Unit Test with memcheck on EL 8.8 completed with status UNSTABLE. https://build.hpdd.intel.com/job/daos-stack/job/daos//view/change-requests/job/PR-13617/3/testReport/

Required-githooks: true
Signed-off-by: Ashley Pittman <[email protected]>
@ashleypittman ashleypittman marked this pull request as ready for review January 16, 2024 16:45
@ashleypittman ashleypittman requested review from a team as code owners January 16, 2024 16:45
@daosbuild1
Copy link
Collaborator

Test stage Unit Test on EL 8.8 completed with status UNSTABLE. https://build.hpdd.intel.com/job/daos-stack/job/daos//view/change-requests/job/PR-13617/4/testReport/

@daosbuild1
Copy link
Collaborator

Test stage Unit Test with memcheck on EL 8.8 completed with status UNSTABLE. https://build.hpdd.intel.com/job/daos-stack/job/daos//view/change-requests/job/PR-13617/4/testReport/

Required-githooks: true
Signed-off-by: Ashley Pittman <[email protected]>
@daosbuild1
Copy link
Collaborator

Test stage Unit Test on EL 8.8 completed with status UNSTABLE. https://build.hpdd.intel.com/job/daos-stack/job/daos//view/change-requests/job/PR-13617/5/testReport/

@daosbuild1
Copy link
Collaborator

Test stage Unit Test with memcheck on EL 8.8 completed with status UNSTABLE. https://build.hpdd.intel.com/job/daos-stack/job/daos//view/change-requests/job/PR-13617/5/testReport/

with open(".build_vars.json", "r") as ofh:
bc = json.load(ofh)

include_dir = f"{bc['PREFIX']}/include"
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
include_dir = f"{bc['PREFIX']}/include"
include_dir = os.path.join(bc['PREFIX'], "include")

include_dir = f"{bc['PREFIX']}/include"

check_dir(include_dir, None)
os.unlink("a.out")
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit. I think it's better to move this unlink to the helper function which generates it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Each compile test will create/overwrite it, removing it on each iteration would be extra work although possibly clearer as you say.

"""Check the whole tree"""

with open(".build_vars.json", "r") as ofh:
bc = json.load(ofh)
Copy link
Contributor

Choose a reason for hiding this comment

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

What does bc stand for? Could we use something more self-documenting?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

build_config? I'll change it to bv to match the filename.

Comment on lines +18 to +19
if not os.path.isfile(os.path.join(h_dir, entry)):
check_dir(include_dir, entry)
Copy link
Contributor

Choose a reason for hiding this comment

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

FYI

Suggested change
if not os.path.isfile(os.path.join(h_dir, entry)):
check_dir(include_dir, entry)
if os.path.isdir(os.path.join(h_dir, entry)):
check_dir(include_dir, entry)

h_dir = os.path.join(include_dir, sub_dir)
for entry in sorted(os.listdir(h_dir)):
if not os.path.isfile(os.path.join(h_dir, entry)):
check_dir(include_dir, entry)
Copy link
Contributor

Choose a reason for hiding this comment

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

This only works recursively once, right? But that's okay because our includes aren't deep

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Correct.

@ashleypittman
Copy link
Contributor Author

Pr has been merged into #13613 but I'll push feedback there.

@ashleypittman ashleypittman deleted the amd/header-check branch January 18, 2024 09:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants