Skip to content

Conversation

rslawson
Copy link
Contributor

Previously the flake would require periodically updating the commit hashes for the submodules (and their submodules, etc.). However, the flake should now handle changes to those without direct user interaction.

@maliberty
Copy link
Member

@donn any concerns here? Nix support is for OL2 so I have none.

@rslawson rslawson force-pushed the nix-remove-commit-hashes branch from 8c358e8 to d306893 Compare March 21, 2025 00:20
@rslawson
Copy link
Contributor Author

My apologies, forgot to update the workflows. Any time the submodules update the flake.lock also needs to be updated, so I think(?) I've done that appropriately here.

@rslawson
Copy link
Contributor Author

rslawson commented Mar 21, 2025

It would also appear that despite trying to make sure that flake.lock stays up-to-date I pushed a broken version of it. I'll push a fix for that shortly.

EDIT: Never mind, something was just odd when I cloned it to another machine - it didn't get the right submodule commits for some reason. In the morning I'll rebase on master to ensure that it works with the project in its current state.

@donn
Copy link

donn commented Mar 21, 2025

This is ORFS, not OpenROAD, but the changes look broadly fine

@rslawson
Copy link
Contributor Author

This is ORFS, not OpenROAD, but the changes look broadly fine

I'm aware, yes. What do you mean by that, though?

@maliberty
Copy link
Member

This is ORFS, not OpenROAD, but the changes look broadly fine

I'm aware, yes. What do you mean by that, though?

I think he is pointing out the OL2 only uses the nix file from OR and not ORFS.

@maliberty
Copy link
Member

@vvbandeira do you understand the scan's error SyntaxWarning: invalid escape sequence '\('? I've never seen that before and there is no recent change.

@rslawson
Copy link
Contributor Author

Forgive my ignorance here, but OL2? If this is in reference to the fact that I added inputs to all the submodules for OpenROAD and YoSys, that's just for reasons of Nix flakes are... interesting at times, to put it kindly. It's a workaround for an issue with submodules in flake dependencies.

@maliberty
Copy link
Member

OL2 = OpenLane2 (https://github.com/efabless/openlane2)

@rslawson
Copy link
Contributor Author

Oh, should I have included that instead of or in addition to OpenROAD in this? If so, I can work on that.

@donn
Copy link

donn commented Mar 21, 2025

@rslawson ORFS and OpenLane 2 are two different implementations of the OpenROAD flow. You're contributing to ORFS right now, so you don't need to worry about OpenLane 2.

If you were contributing to OpenROAD's Nix file, I would indeed need to take a look at that.

@rslawson
Copy link
Contributor Author

Ah, undersood. Thank you for clearing that up for me (:

@vvbandeira
Copy link
Member

@vvbandeira do you understand the scan's error SyntaxWarning: invalid escape sequence '\('? I've never seen that before and there is no recent change.

I think it might be missing the r marker in the strings. I will do some testing and push a PR. But there's an exception to be made because the matching is being done with \dlp which matches 4Lp in the sha256.

Co-authored-by: max <[email protected]>
Signed-off-by: Ryan Slawson <[email protected]>
@rslawson rslawson force-pushed the nix-remove-commit-hashes branch from d306893 to 47709e3 Compare April 9, 2025 14:24
@rslawson
Copy link
Contributor Author

rslawson commented Apr 9, 2025

Hmm, was hoping something would change with rebasing onto current master, but unfortunately no.

@maliberty
Copy link
Member

I added an exclusion for checking flake.lock

Copy link
Member

Choose a reason for hiding this comment

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

Why are there changes to this action and .github/workflows/github-actions-cron-update-yosys.yml ? I don't think they have anything to do with nix.

Copy link
Contributor Author

@rslawson rslawson Apr 9, 2025

Choose a reason for hiding this comment

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

That's because if any of the submodules change, so do their hashes. Then it won't match the one in flake.lock, and so trying to open a dev shell will fail. Users can rectify this by running nix flake update, but I thought it might be nice to update the file automatically in CI whenever OR or Yosys are updated. I have assumed, though, that none of their submodules (both abc submodules, OpenSTA, cxxopts) will never be updated independently of their parents. If that's a bad assumption it may be necessary to add more CI actions.

Copy link
Member

Choose a reason for hiding this comment

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

@vvbandeira does this make sense to include? My interest is spending any effort on nix is quite low so this should be zero hassle to adopt.

Copy link
Member

Choose a reason for hiding this comment

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

I would rather not add Nix to the existing GHAs; this will add a dependency we do not officially support. I am okay with adding a new GHA to test Nix, but it should be noted that it will be the responsibility of Nix users to maintain it, and we do not intend to offer any support for Nix-related problems at this time.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's fair enough, I'll revert those changes and write up something to test the Nix flake. I'll also include an update to documentation somewhere that indicates what to do if building the flake fails.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also, additional question: when you say it's okay to add a new GHA to text Nix, do you mean specifically only that? As in I should not use it to update flake.lock, and instead instruct users to do so?

Copy link
Member

Choose a reason for hiding this comment

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

@rslawson, apologies for the long delay.
You can create a new GHA that creates a draft PR to update the flake.lock; we would accept it.
My concern with adding to the existing PRs is that there could be a case where we want to merge the PR that automates the OR submodule, but for some reason, the GHA is not working/failing due to Nix.

Copy link
Contributor Author

@rslawson rslawson Aug 25, 2025

Choose a reason for hiding this comment

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

@vvbandeira No worries at all, I've had very little time to work on personal projects for a bit now. I'll see about making those changes as soon as I am able.

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.

4 participants