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

idris2.ss: support powerpc #3326

Merged
merged 6 commits into from
Jun 24, 2024
Merged

idris2.ss: support powerpc #3326

merged 6 commits into from
Jun 24, 2024

Conversation

barracuda156
Copy link
Contributor

Description

Fix building on macOS PowerPC

Should this change go in the CHANGELOG?

  • If this is a fix, user-facing change, a compiler change, or a new paper
    implementation, I have updated CHANGELOG_NEXT.md (and potentially also
    CONTRIBUTORS.md).

@buzden
Copy link
Contributor

buzden commented Jun 21, 2024

Every change in the bootstrap file's production procedure needs a change in the release checklist here https://github.com/idris-lang/Idris2/blob/main/Release/CHECKLIST or else it will be just overwritten on the next release

@barracuda156
Copy link
Contributor Author

@buzden Thank you. Could you please take a look and say if that will do? I am not really sure of conventions in that regard.

@CodingCellist
Copy link
Collaborator

CodingCellist commented Jun 21, 2024

@barracuda156 Is this something that needs to be added manually, or can we configure Chez Scheme to consider PowerPC support valid from the get-go? As far as I can tell, there is nothing functionally changing, other than adding the right magic strings to let the pattern match load the library on those architectures as well?

Release/CHECKLIST Outdated Show resolved Hide resolved
@barracuda156
Copy link
Contributor Author

barracuda156 commented Jun 21, 2024

@barracuda156 Is this something that needs to be added manually, or can we configure Chez Scheme to consider PowerPC support valid from the get-go? As far as I can tell, there is nothing functionally changing, other than adding the right magic strings to let the pattern match load the library on those architectures as well?

@CodingCellist Perhaps you can, but I have no idea where that should be done. Locally I had to patch this file to make it work.
If there is some template for it elsewhere, then that is to be fixed.

@dunhamsteve
Copy link
Contributor

dunhamsteve commented Jun 21, 2024

This is only changing the bootstrap file, which is an output of the Idris compiler. That's fine (and necessary) for a one-time fix of bootstrap.

But it's not actually changing the source to the compiler itself. If you recompile Idris with this bootstrap compiler, it will no longer produce compatible programs.

The PR changes CompilerC-45SchemeC-45Chez-schHeader which is the output of compiling schHeader in src/Compiler/Scheme/Chez.idr (around line 98). That is the file needs to be changed for a permanent fix.

After that, I don't think we need the additional checklist item, when the next bootstrap file is generated, it should be built with the updated compiler.

@barracuda156
Copy link
Contributor Author

@dunhamsteve Thank you! For some reason BBEdit did not find that instance for me. I will fix it now and drop checklist.

@barracuda156
Copy link
Contributor Author

@dunhamsteve Is it correct now?

@dunhamsteve
Copy link
Contributor

I think you were doing ChezSep.idr too (I missed that until I looked closer at the .ss patch). I don't know if that's used anymore, but might as well.

@barracuda156
Copy link
Contributor Author

I think you were doing ChezSep.idr too (I missed that until I looked closer at the .ss patch). I don't know if that's used anymore, but might as well.

@dunhamsteve Thank you, added that.

@dunhamsteve
Copy link
Contributor

Looks good to me.

Copy link
Collaborator

@mattpolzin mattpolzin left a comment

Choose a reason for hiding this comment

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

There's one other spot where chez handles the machine type: support/chez/support.ss.

I think it's fine if support is selectively added for Chez, but it might be just as well (and a bit more robust) to also add the same additional machine types for the Racket backed if they are supported by Racket as well.

@CodingCellist
Copy link
Collaborator

@barracuda156
Copy link
Contributor Author

Seems like Racket does indeed support Mac PowerPC: https://github.com/racket/racket/blob/cb410f3e9c349588f8c55aa687aa16585f16e8c1/racket/src/bc/sconfig.h#L647-L660

On a side note, this is slightly incorrect, it seems: it will hardcode the arch to ppc, even if the build is for ppc64 (__POWERPC__ evaluates as true for both).

@barracuda156
Copy link
Contributor Author

@mattpolzin This makes sense, but I need to verify if it actually works. Turns out, there is no port for racket, so I do not have it installed. Let me try writing a port and building it.

@barracuda156
Copy link
Contributor Author

@mattpolzin @dunhamsteve Looks like Racket does not specify arch anywhere besides the bootstrap file? How is it generated?

For chez, I have added ppc to support.ss now.

@barracuda156
Copy link
Contributor Author

Re Racket itself on ppc: it does not look too bad, I almost got it built, but destroot failed with a strip error: racket/racket#5021
Perhaps should be disabled or done with different options.

Copy link
Collaborator

@mattpolzin mattpolzin left a comment

Choose a reason for hiding this comment

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

Let's not worry too much about Racket in this PR given the difficulty with even checking that support for Racket on PowerPC is working.

The PR looks good to me. Thanks!

As a side note, I think there's something fishy with our Racket bootstrap code. It does not appear to be the result of the Racket.idr compiler sourcecode (as noted above)... Nothing to be addressed in this PR. I could also be making the wrong assumption about whether the Racket backend uses any of the Chez backend's code (other than the common scheme files which of course are shared).

@CodingCellist
Copy link
Collaborator

Looks like Racket does not specify arch anywhere besides the bootstrap file? How is it generated?

For chez, I have added ppc to support.ss now.

(and @mattpolzin)

I am also scratching my head here... As far as I can tell, based on the bootstrap files going all the way back to v0.2.0, it seems it just uses the Chez Scheme arch library code-string. Not entirely sure from where, as Compiler.Chez doesn't export its schHeader? Also, how on Earth has the Racket CI ever been passing then?... Currently doing a complete bootstrap of Idris2 backed by Racket only, to see if I can repro the Chez code being present.

But indeed, this is a problem for a different PR. I am also happy with this as-is : )

@CodingCellist
Copy link
Collaborator

I think I've narrowed it down: we include the Chez Scheme header in the Racket bootstrap file because it's derived from building idris2.ipkg and then extracting the resulting build/exec/idris2_app/idris2.rkt, which includes the Chez Scheme compiler because, well, it's part of Idris2 (afaiu this means the self-hosting version technically supports any backend code, just not the ability to run it).

I'm basing this off that afaict, CompilerC-45SchemeC-45Chez-schHeader is only invoked in the compiled Racket file as part of the CompilerC-45SchemeC-45Chez-compileToSS and CompilerC-45SchemeC-45Chez-compileToSSInc functions.

So maybe Racket handles architecture-specific things at the time of building Racket itself and then it is forever part of that install?

@CodingCellist
Copy link
Collaborator

@mattpolzin @barracuda156 do you think it would make sense to revert the changes to the bootstrap files? These will be overwritten with fresh builds on the next release (whenever that may be), and with the other changes from this PR landing, should automatically include support for ppc (at least for the Chez compiler).

@mattpolzin
Copy link
Collaborator

I think it’s worth making the changes to the bootstrap files here or else we would need to wait for a release to actually support bootstrapping on a PowerPC as is the original goal here.

@barracuda156
Copy link
Contributor Author

barracuda156 commented Jun 24, 2024

Anything different from these?
76b02f5
e4f6dc1
It works locally on ppc for me.

UPD. Ah, you were replying to a comment above, didn’t notice.

@mattpolzin mattpolzin merged commit a38f1ac into idris-lang:main Jun 24, 2024
24 checks passed
@barracuda156 barracuda156 deleted the ppc branch June 24, 2024 17:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants