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

dini: Plugin Breaks Test Suite #1713

Closed
sanssecours opened this Issue Dec 3, 2017 · 12 comments

Comments

Projects
None yet
2 participants
@sanssecours
Contributor

sanssecours commented Dec 3, 2017

Description

Unfortunately the latest merge commit to master breaks

.

Proposal

It would be great if we require that everyone opens a pull request before she or he merges non-trivial changes. This way we can check if new code breaks anything. Merging non-working code to master might break the workflow of other developers, who regularly rebase their work on the lastest version of the code base.

@markus2330

This comment has been minimized.

Show comment
Hide comment
@markus2330

markus2330 Dec 3, 2017

Contributor

Thank you for reporting the issue!

It would be great if we require that everyone opens a pull request before she or he merges non-trivial changes. This way we can check if new code breaks anything. Merging non-working code to master might break the workflow of other developers, who regularly rebase their work on the lastest version of the code base.

Sorry, my mistake. Unfortunately, we do not have a workflow to build a Debian package with a new plugin without merging to master first. Thus I merged it without creating a PR. I opened a separate issue for the workflow problem in #1714. Nevertheless, you are right: not breaking the master is more important than being able to build Debian packages sooner. I thought that a new experimental plugin in master is acceptable (even if it does not work) but it seems like my assumption was wrong.

[breaks] the Jenkins build job elektra-ini-mergerequest and

This build job should be changed to use dini instead. But more work is needed until this works.

[breaks] the Travis Linux/ASAN build job

I saw this already but I have not found the problem yet. (And I get different ASAN errors locally, not caused by this PR, so I cannot reproduce.) Might be fixed with 518440b.

Contributor

markus2330 commented Dec 3, 2017

Thank you for reporting the issue!

It would be great if we require that everyone opens a pull request before she or he merges non-trivial changes. This way we can check if new code breaks anything. Merging non-working code to master might break the workflow of other developers, who regularly rebase their work on the lastest version of the code base.

Sorry, my mistake. Unfortunately, we do not have a workflow to build a Debian package with a new plugin without merging to master first. Thus I merged it without creating a PR. I opened a separate issue for the workflow problem in #1714. Nevertheless, you are right: not breaking the master is more important than being able to build Debian packages sooner. I thought that a new experimental plugin in master is acceptable (even if it does not work) but it seems like my assumption was wrong.

[breaks] the Jenkins build job elektra-ini-mergerequest and

This build job should be changed to use dini instead. But more work is needed until this works.

[breaks] the Travis Linux/ASAN build job

I saw this already but I have not found the problem yet. (And I get different ASAN errors locally, not caused by this PR, so I cannot reproduce.) Might be fixed with 518440b.

markus2330 added a commit that referenced this issue Dec 3, 2017

@markus2330

This comment has been minimized.

Show comment
Hide comment
@markus2330

markus2330 Dec 3, 2017

Contributor

Seems like the change introduced a new error (warning):

https://travis-ci.org/ElektraInitiative/libelektra/jobs/310956065

Hopefully fixed in 59d73b0

Contributor

markus2330 commented Dec 3, 2017

Seems like the change introduced a new error (warning):

https://travis-ci.org/ElektraInitiative/libelektra/jobs/310956065

Hopefully fixed in 59d73b0

@markus2330 markus2330 added help wanted and removed proposal labels Dec 3, 2017

@markus2330

This comment has been minimized.

Show comment
Hide comment
@markus2330

markus2330 Dec 3, 2017

Contributor

Proposal accepted for future (thus proposal removed) but ASAN error unfortunately still not fixed (thus help wanted added).

Reproducing is difficult because there are still plenty of ASAN errors in Debian Stretch.

Contributor

markus2330 commented Dec 3, 2017

Proposal accepted for future (thus proposal removed) but ASAN error unfortunately still not fixed (thus help wanted added).

Reproducing is difficult because there are still plenty of ASAN errors in Debian Stretch.

@markus2330 markus2330 referenced this issue Dec 5, 2017

Closed

dini: new default storage plugin #1693

3 of 4 tasks complete
@markus2330

This comment has been minimized.

Show comment
Hide comment
@markus2330

markus2330 Dec 6, 2017

Contributor

Seems like also the testshell_markdown_dini sometimes fails. I added the todos to #1693.

Contributor

markus2330 commented Dec 6, 2017

Seems like also the testshell_markdown_dini sometimes fails. I added the todos to #1693.

@markus2330 markus2330 referenced this issue Dec 8, 2017

Merged

Opmphm changes + new OPMPHM benchmarks #1712

5 of 5 tasks complete
@markus2330

This comment has been minimized.

Show comment
Hide comment
@markus2330

markus2330 Dec 8, 2017

Contributor

@sanssecours What about dropping support for Ubuntu Trusty (14.04)? (Or at least the ASAN build for it.) It is even older than Debian Jessie (April 26th, 2015).

I wonder in which ways travis modified Trusty. (Or is it even using Ubuntu Trusty?) When I bootstrapped trusty I got cmake v2.8, not supported by Elektra anymore. Thus I could not reproduce the ASAN problem.

Contributor

markus2330 commented Dec 8, 2017

@sanssecours What about dropping support for Ubuntu Trusty (14.04)? (Or at least the ASAN build for it.) It is even older than Debian Jessie (April 26th, 2015).

I wonder in which ways travis modified Trusty. (Or is it even using Ubuntu Trusty?) When I bootstrapped trusty I got cmake v2.8, not supported by Elektra anymore. Thus I could not reproduce the ASAN problem.

@sanssecours

This comment has been minimized.

Show comment
Hide comment
@sanssecours

sanssecours Dec 8, 2017

Contributor

@sanssecours What about dropping support for Ubuntu Trusty (14.04)? (Or at least the ASAN build for it.) It is even older than Debian Jessie (April 26th, 2015).

I do not like the idea. I just fixed two memory leaks reported by the Travis ASAN build. However, I agree that Ubuntu Trusty is (horribly) outdated. As far as I can tell Travis should add support for the less outdated Ubuntu Xenial on December 11th 🙌.

I wonder in which ways travis modified Trusty.

The Travis documentation lists some of the modifications. The documentation is quite vague though:

The build-essential metapackage is installed, as well as modern versions of:

  • GCC
  • Clang
  • make
  • autotools
  • cmake
  • scons

. At least I do not really know what version of CMake is considered “modern”.

Contributor

sanssecours commented Dec 8, 2017

@sanssecours What about dropping support for Ubuntu Trusty (14.04)? (Or at least the ASAN build for it.) It is even older than Debian Jessie (April 26th, 2015).

I do not like the idea. I just fixed two memory leaks reported by the Travis ASAN build. However, I agree that Ubuntu Trusty is (horribly) outdated. As far as I can tell Travis should add support for the less outdated Ubuntu Xenial on December 11th 🙌.

I wonder in which ways travis modified Trusty.

The Travis documentation lists some of the modifications. The documentation is quite vague though:

The build-essential metapackage is installed, as well as modern versions of:

  • GCC
  • Clang
  • make
  • autotools
  • cmake
  • scons

. At least I do not really know what version of CMake is considered “modern”.

@markus2330

This comment has been minimized.

Show comment
Hide comment
@markus2330

markus2330 Dec 8, 2017

Contributor

Thank you for the quick reply!

lists some of the modifications

If they exchange core components they should not call it Ubuntu Trusty.

As far as I can tell Travis should add support for the less outdated Ubuntu Xenial on December 11th 🙌.

So lets wait for 3 days and see which ASAN errors we get with Ubuntu Xenial. (Hopefully they provide a real Ubuntu Xenial and not a modified one.)

Contributor

markus2330 commented Dec 8, 2017

Thank you for the quick reply!

lists some of the modifications

If they exchange core components they should not call it Ubuntu Trusty.

As far as I can tell Travis should add support for the less outdated Ubuntu Xenial on December 11th 🙌.

So lets wait for 3 days and see which ASAN errors we get with Ubuntu Xenial. (Hopefully they provide a real Ubuntu Xenial and not a modified one.)

@markus2330

This comment has been minimized.

Show comment
Hide comment
@markus2330

markus2330 Dec 9, 2017

Contributor

Seems like elektra-ini-mergerequest is now fixed. For ASAN multiple problems exist:

https://build.libelektra.org/jenkins/job/elektra-clang-asan/lastFailedBuild/console

Were they all introduced with the dini plugin?

Contributor

markus2330 commented Dec 9, 2017

Seems like elektra-ini-mergerequest is now fixed. For ASAN multiple problems exist:

https://build.libelektra.org/jenkins/job/elektra-clang-asan/lastFailedBuild/console

Were they all introduced with the dini plugin?

@sanssecours

This comment has been minimized.

Show comment
Hide comment
@sanssecours

sanssecours Dec 9, 2017

Contributor

Were they all introduced with the dini plugin?

The tests:

  • testshell_selftest,
  • testshell_markdown_dini and
  • testkdb_allplugins

fail both on Travis and on Jenkins. Since the Travis ASAN build worked before commit 2bac405, I would assume the failures were introduced by dini. Anyway, all of the messages of the form

runtime error: downcast of address … with insufficient space for an object of type 
'_Rb_tree_node<std::pair<const std::basic_string<char>, std::basic_string<char> > >'

show problems in libstdc++ and not in Elektra.

Contributor

sanssecours commented Dec 9, 2017

Were they all introduced with the dini plugin?

The tests:

  • testshell_selftest,
  • testshell_markdown_dini and
  • testkdb_allplugins

fail both on Travis and on Jenkins. Since the Travis ASAN build worked before commit 2bac405, I would assume the failures were introduced by dini. Anyway, all of the messages of the form

runtime error: downcast of address … with insufficient space for an object of type 
'_Rb_tree_node<std::pair<const std::basic_string<char>, std::basic_string<char> > >'

show problems in libstdc++ and not in Elektra.

@sanssecours sanssecours referenced this issue Dec 10, 2017

Merged

Directory Value Plugin & Minor Fixes #1722

7 of 7 tasks complete

@markus2330 markus2330 removed the help wanted label Dec 10, 2017

@markus2330

This comment has been minimized.

Show comment
Hide comment
@markus2330

markus2330 Dec 10, 2017

Contributor

Thank you for fixing it!

Contributor

markus2330 commented Dec 10, 2017

Thank you for fixing it!

@markus2330

This comment has been minimized.

Show comment
Hide comment
@markus2330

markus2330 Dec 10, 2017

Contributor

I thought we found a way to suppress wrongly found ASAN errors. Can't we add the libstdc++ problems to tests/sanitizer.blacklist?

Contributor

markus2330 commented Dec 10, 2017

I thought we found a way to suppress wrongly found ASAN errors. Can't we add the libstdc++ problems to tests/sanitizer.blacklist?

@sanssecours sanssecours referenced this issue Dec 10, 2017

Closed

Release 0.8.21 #1725

12 of 12 tasks complete
@sanssecours

This comment has been minimized.

Show comment
Hide comment
@sanssecours

sanssecours Dec 10, 2017

Contributor

Can't we add the libstdc++ problems to tests/sanitizer.blacklist?

Hm, I never thought about that 😄. I guess you are right. I added an ToDo to issue #1725.

Contributor

sanssecours commented Dec 10, 2017

Can't we add the libstdc++ problems to tests/sanitizer.blacklist?

Hm, I never thought about that 😄. I guess you are right. I added an ToDo to issue #1725.

@sanssecours sanssecours referenced this issue Dec 11, 2017

Merged

Fix Sanitizer Problems #1726

3 of 3 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment