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

Fix in error handling for Docker builds #2249

Merged
merged 5 commits into from
Apr 1, 2024

Conversation

petervdonovan
Copy link
Collaborator

We were always returning false for whether errors occurred in federated docker builds, but we should be checking the context to see whether an error was reported.

We were always returning false for whether errors occurred in federated
docker builds, but we should be checking the context to see whether an
error was reported.
@petervdonovan
Copy link
Collaborator Author

By the way, there is also a problem in DOCKER_RUN_SCRIPT in TestBase. It completed with exit code 0 even though the build failed with a nonzero exit code. I do not want to debug this because I do not want to have to correctly do async fork-join-style programming in bash. Furthermore, the basic idea behind how the script does error handling looks brittle to me because it assumes that the test passes by default, and only fails if a special string is read on stdout. It is preferable to assume that tests fail by default and only mark them as passing if they print a special string, because occasional false positives are preferable to occasional false negatives.

Technically things are OK now because the test framework does not mark failing tests as passing anymore, but maybe I should open an issue about this.

Copy link
Member

@lhstrh lhstrh left a comment

Choose a reason for hiding this comment

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

These changes look great! Thanks for these much-needed fixes, @petervdonovan. Is there a PR in reactor-c that corresponds to the changes included in lf-lang/reactor-c@7427d98...2a3c257?

@lhstrh
Copy link
Member

lhstrh commented Mar 28, 2024

It looks like there are still unaddressed failures also: https://github.com/lf-lang/lingua-franca/actions/runs/8406281187/job/23019999735?pr=2249

@lhstrh lhstrh self-requested a review March 28, 2024 09:28
Note how non-obvious it is that this is a bug or even why the fix works.
This is a consequence of reliance on implicit reliance on big chunks of
shared mutable state. The resource is modified; the context is
computed from the resource; therefore, the resource has to be modified
before the context is computed.
@petervdonovan petervdonovan added this pull request to the merge queue Apr 1, 2024
Merged via the queue into master with commit 22666f0 Apr 1, 2024
24 checks passed
@petervdonovan petervdonovan deleted the fix-docker-error-handling branch April 1, 2024 18:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix docker Issue related to the docker support
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants