Skip to content
This repository has been archived by the owner on Oct 10, 2022. It is now read-only.

Issue 3: HTTP/2 #8

Merged
merged 1 commit into from
Dec 7, 2020
Merged

Issue 3: HTTP/2 #8

merged 1 commit into from
Dec 7, 2020

Conversation

rssen
Copy link
Contributor

@rssen rssen commented Nov 22, 2020

Ref. #3

@rssen rssen requested a review from treiher November 22, 2020 21:39
treiher
treiher suggested changes Nov 23, 2020
View reviewed changes
http_2.rflx Outdated Show resolved Hide resolved
http_2.rflx Outdated Show resolved Hide resolved
http_2.rflx Outdated Show resolved Hide resolved
http_2.rflx Outdated Show resolved Hide resolved
http_2.rflx Outdated Show resolved Hide resolved
http_2.rflx Outdated Show resolved Hide resolved
http_2.rflx Outdated Show resolved Hide resolved
http_2.rflx Outdated Show resolved Hide resolved
http_2.rflx Outdated Show resolved Hide resolved
http_2.rflx Outdated Show resolved Hide resolved
@rssen rssen requested a review from treiher November 24, 2020 15:05
@rssen rssen marked this pull request as ready for review November 24, 2020 15:05
@treiher
Copy link
Member

treiher commented Nov 24, 2020

Currently, the compilability of the generated code is not tested correctly (#9). That's why the failing compilation for the current HTTP/2 specification is not detected (AdaCore/RecordFlux#500). The issue was introduced by the message size invariant in ffb015f.

treiher
treiher suggested changes Nov 26, 2020
View reviewed changes
Copy link
Member

@treiher treiher left a comment

Choose a reason for hiding this comment

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

The changes look fine so far, but three comments of the last review still need some action. Also the feature branch should be rebased to the current main.

@rssen
Copy link
Contributor Author

rssen commented Dec 1, 2020

The tests are now failing. Is that related to #8 (comment)? It does not happen if i check the spec locally.

@treiher
Copy link
Member

treiher commented Dec 1, 2020

The tests are now failing. Is that related to #8 (comment)? It does not happen if i check the spec locally.

Yes, the verification of the specification works fine, but the generated SPARK code is not correct. I will look into that issue in the course of the day.

@treiher treiher changed the title Issue 3 Issue 3: HTTP/2 Dec 3, 2020
treiher
treiher suggested changes Dec 3, 2020
View reviewed changes
Copy link
Member

@treiher treiher left a comment

Choose a reason for hiding this comment

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

Issue AdaCore/RecordFlux#500 is fixed now, the CI checks should not fail anymore. After fixing my small remarks about comments and rebasing to main this PR should be ready to merge.

http_2.rflx Outdated Show resolved Hide resolved
http_2.rflx Outdated Show resolved Hide resolved
rssen
rssen commented Dec 3, 2020
View reviewed changes
http_2.rflx Outdated Show resolved Hide resolved
treiher
treiher suggested changes Dec 4, 2020
View reviewed changes
http_2.rflx Outdated Show resolved Hide resolved
@rssen
Copy link
Contributor Author

rssen commented Dec 4, 2020 •
edited
Loading

Rebased onto main, but there seems to be a problem in the tests with the newly added Settings field.

@treiher
Copy link
Member

treiher commented Dec 4, 2020 •
edited
Loading

Rebased onto main, but there seems to be a problem in the tests with the newly added Settings field.

It seems there is a name conflict in the generated code. Such issues should be detected during model verification. Please create an issue for RecordFlux, preferably with a small reproducer.

treiher
treiher reviewed Dec 4, 2020
View reviewed changes
http_2.rflx Outdated Show resolved Hide resolved
@rssen rssen requested a review from treiher December 4, 2020 13:51
treiher
treiher previously approved these changes Dec 4, 2020
View reviewed changes
@treiher
Copy link
Member

treiher commented Dec 4, 2020

LGTM. The branch still needs to be rebased.

treiher
treiher reviewed Dec 4, 2020
View reviewed changes
http_2.rflx Show resolved Hide resolved
@treiher treiher requested a review from senier December 7, 2020 08:26
@treiher treiher merged commit 9dc02bb into main Dec 7, 2020
@treiher treiher deleted the issue_3 branch December 7, 2020 08:59
@senier senier mentioned this pull request Nov 1, 2022
Open
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@treiher treiher treiher approved these changes

@senier senier senier approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@rssen @treiher @senier