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

ubuntu arm #249

Merged
merged 1 commit into from
Mar 4, 2025
Merged

ubuntu arm #249

merged 1 commit into from
Mar 4, 2025

Conversation

johnnynunez
Copy link
Contributor

This pull request includes updates to the GitHub Actions workflow configuration to add support for Ubuntu ARM architectures. The most important changes include adding new job configurations for Ubuntu ARM and setting up CastXML for these new environments.

Updates to GitHub Actions workflow:

  • .github/workflows/tests.yml: Added job configurations for ubuntu-22.04-arm and ubuntu-24.04-arm with specific settings for architecture, compiler, and Python version.
  • .github/workflows/tests.yml: Added steps to set up CastXML for ubuntu-22.04-arm and ubuntu-24.04-arm environments, including downloading and extracting the appropriate binaries.

@johnnynunez
Copy link
Contributor Author

new bugs appear @iMichka

@iMichka
Copy link
Contributor

iMichka commented Feb 22, 2025

Ok that's 7 errors, some of them I think I already know how to fix (it's mostly badly written test that I wrote a long time ago ...). It will take a few days to go through these errors, I'll open separate pull requests to fix them.

@johnnynunez
Copy link
Contributor Author

Ok that's 7 errors, some of them I think I already know how to fix (it's mostly badly written test that I wrote a long time ago ...). It will take a few days to go through these errors, I'll open separate pull requests to fix them.

Super!

@iMichka
Copy link
Contributor

iMichka commented Feb 26, 2025

Fixed some tests with #250, this can now be rebased

@iMichka
Copy link
Contributor

iMichka commented Feb 26, 2025

I added a fix here to see if this helps. I think there is just one more declaration to remove.

@johnnynunez
Copy link
Contributor Author

I added a fix here to see if this helps. I think there is just one more declaration to remove.

Super. Only know 2 failed

@iMichka
Copy link
Contributor

iMichka commented Feb 26, 2025

Ok, I have extracted that fix to a separate PR to get it shipped ASAP: #251

@johnnynunez
Copy link
Contributor Author

Seems that is not installed. I will check

@iMichka
Copy link
Contributor

iMichka commented Feb 26, 2025

Yes for one of them castxml is not installed.
I'll push a few more commits here to try to debug the other pygccxml bug

@iMichka
Copy link
Contributor

iMichka commented Feb 26, 2025

It's odd, <class 'pygccxml.declarations.namespace.namespace_t'> std [namespace] std has been found twice.
I'll probably have to check the xml file generated by castxml. This will need some more time. I'll continue tomorrow.

If you want to push some more fixes for the missing castxml in the third build, go ahead :)

@johnnynunez
Copy link
Contributor Author

there was a typo error @iMichka fixed. It should build for 3 arm os

@johnnynunez
Copy link
Contributor Author

It's odd, <class 'pygccxml.declarations.namespace.namespace_t'> std [namespace] std has been found twice. I'll probably have to check the xml file generated by castxml. This will need some more time. I'll continue tomorrow.

If you want to push some more fixes for the missing castxml in the third build, go ahead :)

Done!

@iMichka iMichka force-pushed the ubuntu-arm branch 2 times, most recently from 6478480 to 92bbcbb Compare February 28, 2025 22:52
@iMichka
Copy link
Contributor

iMichka commented Feb 28, 2025

Here is the xml file based on the CI output https://gist.github.com/iMichka/1b35b9b8c3ac40c59b8e0c0094715971

<Namespace id="_3536" name="std" context="_1"/>
<Namespace id="_66" name="std" context="_1" ....

_3536 is related to __va_list, so that's intriguing

 <Struct id="_1102" name="__va_list" context="_3536" location="f0:0" file="f0" 

@iMichka iMichka force-pushed the ubuntu-arm branch 2 times, most recently from d0e2fd7 to b318730 Compare March 2, 2025 21:24
@johnnynunez
Copy link
Contributor Author

johnnynunez commented Mar 3, 2025

I've try everything in my fork, and I didn't find it where is the problem @iMichka

I investigate it, and file by file for me is working with this code:

@staticmethod
    def _join_top_namespaces(main_ns_list, other_ns_list):
        answer = main_ns_list[:]
        for other_ns in other_ns_list:
            # Instead of find_declaration(...), gather ALL that match name
            same_name_namespaces = pygccxml.declarations.find_all_declarations(
                answer,
                decl_type=pygccxml.declarations.namespace_t,
                name=other_ns.name,
                parent=None,  # top-level only
                recursive=False
            )
            if len(same_name_namespaces) == 0:
                answer.append(other_ns)
            elif len(same_name_namespaces) == 1:
                same_name_namespaces[0].take_parenting(other_ns)
            else:
                primary_ns = same_name_namespaces[0]
                for extra_ns in same_name_namespaces[1:]:
                    primary_ns.take_parenting(extra_ns)
                    answer.remove(extra_ns)
                # then unify the new other_ns
                primary_ns.take_parenting(other_ns)

        return answer

I've seen that all tests are working:
"global_ns_fixture_file_by_file1",
"global_ns_fixture_file_by_file2",
image

the problem are:
"global_ns_fixture_all_at_once1",
"global_ns_fixture_all_at_once2",

@iMichka
Copy link
Contributor

iMichka commented Mar 3, 2025

Found it, the same change was missing from __parse_all_at_once.

Looks like the join logic was never really tested / we did not expect this edge case.

This pull request will need a big cleanup. I'll have some time tomorrow evening to move this forward. We are close to be done here :)

Regarding the new implementation of _join_top_namespaces. I see what you did here, it "works" but I feel this can refactored even more. The logic is ... complex to understand. There is a loop with multiple conditions, using append/remove on a list.
I am wondering if now that we use pygccxml.declarations.find_all_declarations, we could not simplify this even more.

@johnnynunez
Copy link
Contributor Author

Found it, the same change was missing from __parse_all_at_once.

Looks like the join logic was never really tested / we did not expect this edge case.

This pull request will need a big cleanup. I'll have some time tomorrow evening to move this forward. We are close to be done here :)

Regarding the new implementation of _join_top_namespaces. I see what you did here, it "works" but I feel this can refactored even more. The logic is ... complex to understand. There is a loop with multiple conditions, using append/remove on a list. I am wondering if now that we use pygccxml.declarations.find_all_declarations, we could not simplify this even more.

Thanks a lot!

@johnnynunez
Copy link
Contributor Author

Found it, the same change was missing from __parse_all_at_once.

Looks like the join logic was never really tested / we did not expect this edge case.

This pull request will need a big cleanup. I'll have some time tomorrow evening to move this forward. We are close to be done here :)

Regarding the new implementation of _join_top_namespaces. I see what you did here, it "works" but I feel this can refactored even more. The logic is ... complex to understand. There is a loop with multiple conditions, using append/remove on a list. I am wondering if now that we use pygccxml.declarations.find_all_declarations, we could not simplify this even more.

I can clean it with your fixes.

@johnnynunez
Copy link
Contributor Author

@iMichka cleaned

@johnnynunez
Copy link
Contributor Author

johnnynunez commented Mar 4, 2025

@iMichka could you merge this branch first, and then create optimizations in other branches?
Because with this branch, now libraries like OMPL that there was that bug from years, now it is possible to build thanks to your work

image

@johnnynunez johnnynunez mentioned this pull request Mar 4, 2025
@iMichka iMichka merged commit b56397d into CastXML:develop Mar 4, 2025
19 checks passed
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.

2 participants