Skip to content

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

Open
ana-pantilie wants to merge 8 commits into
masterfrom
ana/add-readme-certificates
Open

Certifier: bundle README.md with generated Agda certificate#7748
ana-pantilie wants to merge 8 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
Copy link
Copy Markdown
Member

@zliu41 zliu41 left a comment

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
Copy link
Copy Markdown
Member

zliu41 commented May 5, 2026

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
Copy link
Copy Markdown
Member

@zliu41 zliu41 left a comment

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.

Copy link
Copy Markdown
Member

@zliu41 zliu41 left a comment

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
Copy link
Copy Markdown
Member

zliu41 commented May 8, 2026

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
Copy link
Copy Markdown
Member

zliu41 commented May 12, 2026

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.

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