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

Fix issue with build using flake system in issue #747 #754

Merged
merged 3 commits into from
Mar 18, 2024

Conversation

redanaheim
Copy link
Contributor

See issue #747

Overview of changes:

  • Added two lines to gitignore
  • Removed vscode:prepublish script from package.json because it causes the flake build script to attempt (and fail) to install the yarn dependencies using yarn when building the .vsix
  • Added yarn.nix files generated using yarn2nix that allow building the yarn dependencies using nix
  • Added correct version coq derivations to nativeBuildInputs for each version of vscoq-language-server so that coqc is accessible during the language server build process
  • Added to buildPhase of vscoq-client: first build UI using scripts in package.json through yarn, then webpack the whole thing, vsce package, then move the .vsix to the bin folder.

One possible problem: I changed the commit the coq-master dependency is derived from, which (according to the comment) may break something in the CI workflow. I'm not sure what the pinned version is, but the latest commits on coq/coq don't build with the flake system.

These changes were tested on aarch64-darwin (MacOS 14.1, nix 2.20.3) and aarch64-linux (nixOS, nix 2.18.1).

- Make the project build using flake
- Revert to version of coq that builds without dune version error
- Update flake.nix
@gares
Copy link
Member

gares commented Mar 13, 2024

Added yarn.nix files generated using yarn2nix that allow building the yarn dependencies using nix

Thanks!

Is there a way to run this tool in CI and check that the .nix is up to date wrt the yarn one? I'm not a nix user, neither is Romain AFAIK, so CI better be rock solid on nix, or we won't notice these issues timely.

@redanaheim
Copy link
Contributor Author

Is there a way to run this tool in CI and check that the .nix is up to date wrt the yarn one?

Yeah, using the following command will just create a file called new_yarn.nix using the yarn.lock in the current directory (assuming nix is available at the relevant point in CI):

nix-shell -p yarn2nix --run "yarn2nix > new_yarn.nix"

Then, you could compare hashes with the committed file (yarn.nix) and error if they're not the same.

Alternatively, you could overwrite the yarn.nix with that same command, which would produce an error when building the flake because flake.lock would be out of date if the contents of yarn.nix aren't the same. That said, I'm not sure if the flake is actually built in the CI. It seems like only nix develop is used.

@@ -731,7 +731,6 @@
"start:search-ui": "cd search-ui && yarn run start",
"build:search-ui": "cd search-ui && yarn run build",
"build:dev:search-ui": "cd search-ui && yarn run build:dev",
"vscode:prepublish": "yarn run package",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Removing this causes the publish-extension CI job to fail. Maybe we can just add a yarn run package step to the CI, before the relevant job ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks again for you contribution :-)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe we can just add a yarn run package step to the CI

Yeah, that's kind of what I was envisioning. Seems like that's the only reason CI failed

@rtetley
Copy link
Collaborator

rtetley commented Mar 15, 2024

Took the liberty of attempting a fix

@rtetley
Copy link
Collaborator

rtetley commented Mar 15, 2024

Argh, fixed the wrong file... Also realised retrospectively that it would have been more polite to ask for permission before pushing to the main branch of your fork. Would you mind if I push the fix on the right file @redanaheim ?

@redanaheim
Copy link
Contributor Author

@rtetley No yeah it's fine go ahead, I didn't even know you could do that lol

@rtetley
Copy link
Collaborator

rtetley commented Mar 15, 2024

Haha yeah, you can opt out of it when you create the PR afaik

@redanaheim
Copy link
Contributor Author

@rtetley @gares do you want me to try to implement the CI check for whether each yarn.nix is up to date? I'm thinking

nix-shell -p yarn2nix --run "yarn2nix > new_yarn.nix"
diff --brief new_yarn.nix yarn.nix

run in each directory with a yarn.nix.

Since the vscode:prepublish action was deleted, we do a yarn run package before
launching the relevant actions
@rtetley
Copy link
Collaborator

rtetley commented Mar 15, 2024

@redanaheim sounds good ! Please do :-) I'm really not that much of a nix expert...

@rtetley
Copy link
Collaborator

rtetley commented Mar 15, 2024

Okay finally looks like this should work. One little nitpick: the PIN_COQ needs to track the master branch of coq, it allows for coq developers to do PRs which keep the language server up to date with the latest coq api. What is the issue with the old pin ? It was probably currently not up to date.

@redanaheim
Copy link
Contributor Author

redanaheim commented Mar 15, 2024

What is the issue with the old pin ? It was probably currently not up to date.

@rtetley There is no issue, I changed it on my own fork to be the latest version that builds with flakes. I can revert it back to the version currently on main if you'd like.

cd to correct directories this time

"diff" exits with exit code 1 iff the files aren't the same
@rtetley
Copy link
Collaborator

rtetley commented Mar 15, 2024

What is the issue with the old pin ? It was probably currently not up to date.

@rtetley There is no issue, I changed it on my own fork to be the latest version that builds with flakes. I can revert it back to the version currently on main if you'd like.

Ah okay cool ! Nah that sounds fine ! Thanks !

@rtetley rtetley merged commit 19daf9d into coq:main Mar 18, 2024
16 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.

3 participants