Skip to content

Certifier: bundle README.md with generated Agda certificate - #7748

Merged
zliu41 merged 7 commits into
masterfrom
ana/add-readme-certificates
May 13, 2026
Merged

Certifier: bundle README.md with generated Agda certificate#7748
zliu41 merged 7 commits into
masterfrom
ana/add-readme-certificates

Conversation

@ana-pantilie

Copy link
Copy Markdown
Contributor

@ana-pantilie
ana-pantilie marked this pull request as ready for review April 30, 2026 17:39

@zliu41 zliu41 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

All these backslashes makes this quite hard to maintain. This is mostly static text, the only dynamic part is name, but I don't think you really need it to be dynamic. You can just make a static readme and load it with file-embed.

@zliu41

zliu41 commented May 5, 2026

Copy link
Copy Markdown
Member

Even if you really want the dynamic name, you can easily do a Text.replace.

@ana-pantilie
ana-pantilie requested a review from zliu41 May 6, 2026 13:30

@zliu41 zliu41 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'm not sure if data-files works. @zeme-wana What do you think? You mentioned this before about data-files:

They do in CI. But once the executable is built and deployed to some machine, the files are nowhere to be found

In any case, this is exactly what file-embed is designed to do, and that definitely works.

@zliu41 zliu41 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yeah, since we release uplc as a single executable, this can't work. You need to use file-embed.

@ana-pantilie
ana-pantilie force-pushed the ana/add-readme-certificates branch from 1967f3c to 96add23 Compare May 7, 2026 16:32
@ana-pantilie

Copy link
Copy Markdown
Contributor Author

@zliu41 I'm now getting https://ci.iog.io/build/12767781/nixlog/18 which I'm unable to reproduce locally.

@zeme-wana said that there might be platform specific issues with this approach, so I'm going to just use the original version, which was to embed the file into a Haskell string. It's much simpler, the file isn't that large, we won't have to change it too often and I think we've already spent too much time on this.

@ana-pantilie
ana-pantilie requested a review from zliu41 May 8, 2026 11:07
@zliu41

zliu41 commented May 8, 2026

Copy link
Copy Markdown
Member

platform specific issues

What platform issues - can you elaborate? We use TH to load the cost model JSON files and there's no problem with that.

@ana-pantilie

Copy link
Copy Markdown
Contributor Author

@zliu41 The message I got from @zeme-wana regarding platform specific issues is that it might break CI on Windows. I did not get to see if this error would happen or not, because I got a different error (https://ci.iog.io/build/12767781/nixlog/18) which I also mentioned in the previous message. I wasn't able to reproduce this error locally, and it seemed to me like, given the issue and other potential issues, we should opt for the simplest solution.

@zliu41

zliu41 commented May 12, 2026

Copy link
Copy Markdown
Member

I did not get to see if this error would happen or not, because I got a different error

Your error is because you didn't add the md file to extra-source-files.

it might break CI on Windows

@zeme-wana this is no longer a problem, which is why we decided to keep readJSONFromFile, right? And file-embed is also much less likely to cause the same problem than readJSONFromFile.

@zeme-wana

Copy link
Copy Markdown
Collaborator

Yes, @zliu41 is right about the error. Worth trying with extra-source-files first. It should work, even when xcompiling on windows.

@ana-pantilie

Copy link
Copy Markdown
Contributor Author

Your error is because you didn't add the md file to extra-source-files.

I completely forgot about that, sorry 🤦‍♀️

Yes, @zliu41 is right about the error. Worth trying with extra-source-files first. It should work, even when xcompiling on windows.

👍

@ana-pantilie
ana-pantilie force-pushed the ana/add-readme-certificates branch from a92017f to 027c2cf Compare May 13, 2026 13:40
@zliu41
zliu41 merged commit 772a9f3 into master May 13, 2026
9 checks passed
@zliu41
zliu41 deleted the ana/add-readme-certificates branch May 13, 2026 20:07
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.

3 participants