search

mouse07410 / asn1c

The ASN.1 Compiler
http://lionet.info/asn1c/
BSD 2-Clause "Simplified" License
94 stars 70 forks source link

Oai on vlm master #103

Closed ruffyontheweb closed 1 year ago

ruffyontheweb commented 2 years ago

A rebase of changes from https://gitlab.eurecom.fr/oai/asn1c/-/tree/velichkov_s1ap_plus_option_group onto vlm_master abd1faa6cf396ab1a4cddbe637f80316aa2cdef0. I hand checked that changes were applied. Caveat: in the skeletons directory some changes were not applied due to refactor that split files in the skeletons directory. There is a non-trivial diff set from the skeletons directory, but not knowing the implications of the changes I kept the code from vlm_master. It appears that this rebase and velichkov's branch have the same behavior in functional tests of the open air interface project which depends on asn1c.

ruffyontheweb commented 2 years ago

Looks like the PR broke something. Log file attache test-suite.log d

mouse07410 commented 2 years ago

Yes, it broke the main test-suite.

Would you have time to investigate? Presumably, compare the files containing expected output with what asn1c products?

ruffyontheweb commented 2 years ago

Yes, I will at least try to find the offending commit.

ruffyontheweb commented 2 years ago

Yes, I will at least try to find the offending commit.

Rolling back to (checking out) 582f241b3c99a635b78e7fdc1056c9a3a944cf72 seems to allow the failing tests to pass, but reverting the next commit, 7bc61166cc7ca3a3dfa4dc571d23a4fd9b221413, onto head does not seem to fix the issue.

mouse07410 commented 2 years ago

Yes, I will at least try to find the offending commit.

Thank you!

As a suggestion, if a "partial PR" improves something and does not break the tests, perhaps split this PR in two? @pespin, @velichkov, any opinion?

pespin commented 2 years ago

Yes, it probably makes sense to split it into several separated PR as much as possible.

ruffyontheweb commented 2 years ago

I can't seem to localize a single commit that breaks the test.

I need to divert to something else right now, but hope to try again. I'd really love to bring OAI up to something that passes make check and also generates 3GPP's (admittedly complex) definitions.

mouse07410 commented 2 years ago

I can't seem to localize a single commit that breaks the test.

Perhaps, if stuff up to and including https://github.com/mouse07410/asn1c/commit/582f241b3c99a635b78e7fdc1056c9a3a944cf72 seems to work, and is beneficial - maybe you create a PR comprised only of f038e78 and https://github.com/mouse07410/asn1c/commit/582f241b3c99a635b78e7fdc1056c9a3a944cf72 ?

And keep working on the rest, as your time allows?

Also, please give a thought to the idea: is the test failing because the output format/content changed?

I'd really love to bring OAI up to something that passes make check and also generates 3GPP's (admittedly complex) definitions.

So would I.

ruffyontheweb commented 2 years ago

I can't seem to localize a single commit that breaks the test.

Perhaps, if stuff up to and including 582f241 seems to work, and is beneficial - maybe you create a PR comprised only of f038e78 and 582f241 ?

And keep working on the rest, as your time allows?

PR #105 created

Also, please give a thought to the idea: is the test failing because the output format/content changed?

This seems like a very reasonable place to begin investigating, but really my asn1c knowledge is not so strong. Maybe a place to start is just to diff the outputs and see what has changed in the generated code/tests.

ljbade commented 1 year ago

Worth noting that the skeleton file changes from the branch you mentioned you had to drop already seem to be in master

ljbade commented 1 year ago

Looking at the test failures, the first one seems to be from https://github.com/ruffyontheweb/asn1c/commit/7bc61166cc7ca3a3dfa4dc571d23a4fd9b221413 - it adds some extra asn_constant.h lines that are not in the test files

ljbade commented 1 year ago

running ./check-parsing.sh regenerate in tests/tests-asn1c-compiler updates the test output files to allow you to check for changes with git diff

doing this confirms most of them are from https://github.com/ruffyontheweb/asn1c/commit/7bc61166cc7ca3a3dfa4dc571d23a4fd9b221413

however the file 160-multiple-parameterized-instance-OK.asn1.+-P_-gen-UPER_-gen-APER_-fcompound-names has a change as well

ljbade commented 1 year ago

by regenerating the files for each commit 1 by 1, I traced the issue with 160-multiple-parameterized-instance-OK.asn1.+-P_-gen-UPER_-gen-APER_-fcompound-names to https://github.com/mouse07410/asn1c/pull/103/commits/25e65c01b4b3cf60e5445f4e89338e5fbf654ad4

ljbade commented 1 year ago

Made a new PR to fix the tests - https://github.com/mouse07410/asn1c/pull/111

ruffyontheweb commented 1 year ago

Worth noting that the skeleton file changes from the branch you mentioned you had to drop already seem to be in master

Yes! I seem to recall that was the case when I did a head to head. Thanks for verifying!!

ruffyontheweb commented 1 year ago

Hi all! I'm thrilled that these changes have been pulled into vlm_master with https://github.com/mouse07410/asn1c/pull/111. Is the proper thing to do to close this PR?

mouse07410 commented 1 year ago

Hi all! I'm thrilled that these changes have been pulled into vlm_master with https://github.com/mouse07410/asn1c/pull/111. Is the proper thing to do to close this PR?

Correct. We don't need two PRs doing the same thing.

You're happy with how #111 addressed your needs?

ruffyontheweb commented 1 year ago

Hi all! I'm thrilled that these changes have been pulled into vlm_master with #111. Is the proper thing to do to close this PR?

Correct. We don't need two PRs doing the same thing.

You're happy with how #111 addressed your needs?

Yes, all commits in this PR that didn't make it into vlm_master with PR #105 have been merged into vlm_master on #111.

I've emailed the OAI developer's list notifying them of this update. https://lists.eurecom.fr/sympa/arc/openair5g-devel/2022-11/msg00007.html