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

Issue 4577 - Add GitHub actions #4625

Closed
wants to merge 1 commit into from
Closed

Issue 4577 - Add GitHub actions #4625

wants to merge 1 commit into from

Conversation

vashirov
Copy link
Member

@vashirov vashirov commented Feb 16, 2021

Description:

  • Add ASAN variant for tests
  • Add HTML report for pytest, that contains ASAN/LSAN reports

Relates: #4577

Reviewed by:

@vashirov
Copy link
Member Author

It doesn't show all checks because I split the workflows. And for some (security?) reasons GitHub doesn't execute additional workflows that are not yet merged. Only modified workflows. I will post a link with a full run from my fork.

@vashirov
Copy link
Member Author

@vashirov
Copy link
Member Author

Hmm, switching to F32 didn't help, test artifacts are still not useful. I will set this to WIP until I fix ASAN reports.

@vashirov vashirov added the work in progress Work in Progress - can be reviewed, but not ready for merge. label Feb 16, 2021
@vashirov vashirov marked this pull request as draft February 16, 2021 10:08
@vashirov
Copy link
Member Author

Ok, now there are memory leaks reports. But I have another problem now and need your opinion.
With fast_unwind_on_malloc=1 the traces are pretty much useless. They just indicate that there is a leak, but not print a proper stack trace.
With fast_unwind_on_malloc=0 tests run 3-4 times longer and sometimes hang. So for PR checks it's not ideal.

I propose to run with fast_unwind_on_malloc=1 on PRs and have a nightly run (which is not time sensitive) with 0.
WDYT?

@vashirov
Copy link
Member Author

Forgot the link: https://github.com/vashirov/389-ds-base/actions/runs/574633583
Check artifacts at the bottom. Archives with -asan have html page which has links to ASAN/LSAN reports for exact test that generated them.

@vashirov vashirov removed the work in progress Work in Progress - can be reviewed, but not ready for merge. label Feb 17, 2021
@vashirov vashirov marked this pull request as ready for review February 17, 2021 19:02
@Firstyear
Copy link
Contributor

I think we can have fast_unwind_on_malloc=1, because knowing that any test has failed indicates to us to investigate further or to look at the nightly which does have the ability to generate the exact report. So I'm okay with this :)

Copy link
Member

@droideck droideck left a comment

Choose a reason for hiding this comment

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

Looks good to me!

Description:

* Add ASAN variant for tests
* Add HTML report for pytest, that contains ASAN/LSAN reports

Relates: #4577

Reviewed by: @Firstyear, @droideck (Thanks!)
@@ -13,7 +14,15 @@
suites += [repl_test.replace('dirsrvtests/tests/suites/', '') for repl_test in repl_tests]
suites.sort()

suites_list = [{ "suite": suite} for suite in suites]
matrix = {"include": suites_list}
variants = ['gcc', 'gcc-asan']
Copy link
Contributor

Choose a reason for hiding this comment

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

gcc-asan doesn't work with rust, so we can't use this. We need clang-asan to be consistent, especially so to match the correct llvm versions.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, I will replace gcc-asan with clang-asan.

Copy link
Member Author

Choose a reason for hiding this comment

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

@Firstyear, clang with -fsanitize=address fails to compile: https://github.com/vashirov/389-ds-base/runs/3554323865#step:6:3541
Any ideas?

Copy link
Member Author

Choose a reason for hiding this comment

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

I found that I filed an issue for this some time ago #4596
I feel that clang-asan would require more efforts than I expected, so I'd like to to keep gcc-asan for now and add clang-asan in a follow up PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

Didn't we change how lto was managed here to make it work? I'm building and using clang-asan just fine?

The issue with gcc-asan is that I've noticed more and more that gcc asan and rust's llvm backed asan aren't compatible and can not be mixed, which has led to failures to build. That's why I've been using clang now so that they both have the same llvm backend and asan versions.

Copy link
Member Author

Choose a reason for hiding this comment

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

No, we didn't :( I couldn't make LTO builds with clang and then I switched to other things, forgetting about the issue.
I need some help from you regarding Rust + LTO, I will put the details in the related issue.

I haven't encountered any failures myself. Ideally I'd like to have the full matrix (gcc, gcc-asan, clang, clang-asan), so that we can catch these build issues too. But I'm hitting GH actions limit of 256 jobs in the matrix. I have some ideas how to circumvent this with composable actions, will try it next week.

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay, I'll look at the LTO issue later, but I think we can't do gcc-asan + rust at all. I've had so many conflicts between the two ASAN apis as they exist, and a bunch of false positives at this point, that I don't think it should be considered reliable.

@vashirov vashirov closed this by deleting the head repository Jan 24, 2024
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.

3 participants