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

Added portable_minimal_ada_rts crate #940

Merged
merged 1 commit into from
Dec 18, 2023

Conversation

jgrivera67
Copy link
Contributor

No description provided.

@mosteo
Copy link
Member

mosteo commented Dec 15, 2023

The dreaded trouble with wrong line terminators strikes again. At first sight, you have the proper compiler flags, so I'm not sure what's going on right now.

Comment on lines 11 to 12
[[depends-on]]
gnat_arm_elf = "^12.1.2"
Copy link
Member

Choose a reason for hiding this comment

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

Is this dependency on ARM intended?

Copy link
Contributor Author

@jgrivera67 jgrivera67 Dec 15, 2023

Choose a reason for hiding this comment

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

The code of this crate itself should be platform independent, but I needed to specify a compiler to be able to pass the alire-index PR checks. The client code of this crate, which lives in another crate, does depend on ARM. However,
this crate should be compilable for any platform.

What would be a cleaner way to specify a compiler dependency for this minimal Ada RTS crate, given the fact it should compile on any platform?

Copy link
Member

Choose a reason for hiding this comment

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

A compiler dependency shouldn't be necessary, and given what you tell me, it isn't desirable as clients of your RTS may want to specify a different target. What was the problem without it?

Copy link
Contributor Author

@jgrivera67 jgrivera67 Dec 15, 2023

Choose a reason for hiding this comment

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

I get this error:
"Note: Building portable_minimal_ada_rts/portable_minimal_ada_rts.gpr...
gprconfig: can't find a toolchain for the following configuration:
gprconfig: language 'ada', target 'arm-eabi', runtime '/Users/runner/work/alire-index/alire-index/portable_minimal_ada_rts_1.0.0_99f278cb'
Setup
portable_minimal_ada_rts.gpr:11:25: warning: libraries are not supported on this platform
[mkdir] object directory for project Portable_Minimal_Ada_Rts
portable_minimal_ada_rts.gpr:4:09: no compiler for language "Ada", cannot compile "portable_minimal_ada_rts_config.ads"
[mkdir] library directory for project Portable_Minimal_Ada_Rts
gprbuild: *** compilation phase failed
ERROR: Command ["gprbuild", "-s", "-j0", "-p", "-P", "/Users/runner/work/alire-index/alire-index/portable_minimal_ada_rts_1.0.0_99f278cb/portable_minimal_ada_rts.gpr"] exited with code 4
stderr: Command ["gprbuild", "-s", "-j0", "-p", "-P", "/Users/runner/work/alire-index/alire-index/portable_minimal_ada_rts_1.0.0_99f278cb/portable_minimal_ada_rts.gpr"] exited with code 4
ERROR: Compilation failed.
stderr: ALIRE.CHECKED_ERROR"

I wonder if when building an RTS (-gnatg switch), an explicit toolchain needs to be specified.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried adding a dependency on gnat, but the build for macos-latest fails with the following error:

Error: nodename nor servname provided, or not known (api.github.com:443)

Copy link
Member

Choose a reason for hiding this comment

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

Mm. An explicit gnat shouldn't also be needed. I guess it's possible that, when compiling an RTS, things are a little out of the ordinary , but I lack the expertise wrt to cross-compiling. @Fabien-Chouteau will problably know.

gprconfig: language 'ada', target 'arm-eabi',

That suggests to me that, even if you removed the dependency in the Alire manifest, there's a target set in some .gpr, which defeats the purpose.

If, once you make sure no references to targets are anywhere, the tests still fail, what you can do is create a test crate inside a test folder, that uses this crate through a ".." pin. That crate may use an explicit target. Then, CI checks will simply build that crate instead of this one. Although I'd like to see the fail log in that case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, you were right. I had a target clause in the .gpr file, which did not need to be there. I removed it and cleaned some warnings for Windws buids and I'm passing all 15 checks without needing the depedency on a specific compiler.

@jgrivera67 jgrivera67 requested a review from mosteo December 15, 2023 16:04
@jgrivera67
Copy link
Contributor Author

The dreaded trouble with wrong line terminators strikes again. At first sight, you have the proper compiler flags, so I'm not sure what's going on right now.

Thanks for catching this @mosteo. @Fabien-Chouteau kindly told me how to fix it:

"You are using the -gnatg switch here:
https://github.com/jgrivera67/portable_minimal_ada_rts/blob/a66019438515d482f8d12d6c9fe4003fe8f4327f/portable_minimal_ada_rts.gpr#L30

That's good because this switch is required to build a run-time project, however -gnatg also enables other switches (see here), in particular style checks that revert what you have set in ADAFLAGS. That's why you have build errors on Windows with the line termination warning. So you have to set -gnatg before the ADAFLAGS switches."

Now, my PR passes all 15 checks

@jgrivera67 jgrivera67 force-pushed the stable-1.2.1 branch 15 times, most recently from 125e5f2 to bda6049 Compare December 17, 2023 02:58
@mosteo mosteo merged commit 453f25b into alire-project:stable-1.2.1 Dec 18, 2023
15 checks passed
@mosteo
Copy link
Member

mosteo commented Dec 18, 2023

Thanks, merged.

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