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

Remove skip-validate option #1749

Merged
merged 3 commits into from
Jan 22, 2024
Merged

Remove skip-validate option #1749

merged 3 commits into from
Jan 22, 2024

Conversation

joonho3020
Copy link
Contributor

@joonho3020 joonho3020 commented Jan 22, 2024

The default installation option should not ask "y/n".
I think most people would want to run build-setup.sh and walk away 🥲

Related PRs / Issues:

Type of change:

  • Bug fix
  • New feature
  • Other enhancement

Impact:

  • RTL change
  • Software change (RISC-V software)
  • Build system change
  • Other

Contributor Checklist:

  • Did you set main as the base branch?
  • Is this PR's title suitable for inclusion in the changelog and have you added a changelog:<topic> label?
  • Did you state the type-of-change/impact?
  • Did you delete any extraneous prints/debugging code?
  • Did you mark the PR with a changelog: label?
  • (If applicable) Did you update the conda .conda-lock.yml file if you updated the conda requirements file?
  • (If applicable) Did you add documentation for the feature?
  • (If applicable) Did you add a test demonstrating the PR?
  • (If applicable) Did you mark the PR as Please Backport?

@jerryz123
Copy link
Contributor

I personally agree with this change

@jerryz123
Copy link
Contributor

It's probably a good idea to leave the flags as no-ops so people's existing flows and documentation won't all immediately break.

@joonho3020
Copy link
Contributor Author

It's probably a good idea to leave the flags as no-ops so people's existing flows and documentation won't all immediately break.

Hmmm.... but then we need to cleanup the "no-op" flags later...

@jerryz123
Copy link
Contributor

It's probably a good idea to leave the flags as no-ops so people's existing flows and documentation won't all immediately break.

Hmmm.... but then we need to cleanup the "no-op" flags later...

Yeah, at some point. But IMO this is an instance where we should prioritize minimizing user pain.

@joonho3020
Copy link
Contributor Author

There are times when radical changes just have to take place.......

@jerryz123
Copy link
Contributor

There are times when radical changes just have to take place.......

Removing the flag and all relevant documentation is fine.

Leaving behind the parsing as a no-op is a minimal QoL feature that has no impact otherwise.

@joonho3020
Copy link
Contributor Author

Hmm.. I think it is even more confusing to users if there is a "flag that does nothing".

@joonho3020
Copy link
Contributor Author

😢

@jerryz123
Copy link
Contributor

Remove it from the documentation and help text. Users should not be able to discover it and be confused.

It would only affect existing users following old docs, or with their own scripts.

@SeahK
Copy link
Contributor

SeahK commented Jan 22, 2024

Oh, this change seems good. I sometimes forget to do Y and waste an hour.

@T-K-233
Copy link
Member

T-K-233 commented Jan 22, 2024

Nice change. I will update the tutorials when we do a version release with this change.

@T-K-233 T-K-233 mentioned this pull request Jan 22, 2024
16 tasks
@jerryz123
Copy link
Contributor

I'd like to wait for @abejgonzalez and @sagark before merge.

Copy link
Contributor

@jerryz123 jerryz123 left a comment

Choose a reason for hiding this comment

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

Wait for @abejgonzalez and @sagark

@joonho3020
Copy link
Contributor Author

Release has to happen...

@jerryz123
Copy link
Contributor

I know, but as others have asked for this feature in the past, it would be rude to unilaterally remove it without their feedback.

@abejgonzalez
Copy link
Contributor

abejgonzalez commented Jan 22, 2024

This is going to get rejected by @sagark most likely. This flag being there exists strictly for new users who accidentally don't check out a release to have some sort of prompt telling them "Hey this isn't a release, this may not work". I'm personally indifferent about this now.

@jerryz123
Copy link
Contributor

If @sagark concedes then we can merge this, after my backwards-compatibility concerns are addressed. I think we certainly have some consensus that this is the right move.

FWIW, this isn't the Wild West of rocket-chip and RTL changes between releases anymore... at this point, most changes are strictly QoL improvements or new-features. The latest commit on main is much more likely to be usable than the latest release, and we will continue to only provide support for the latest main.

Thus, I don't see the benefit of pushing users into using tagged releases.

@sagark
Copy link
Member

sagark commented Jan 22, 2024

FWIW, this isn't the Wild West of rocket-chip and RTL changes between releases anymore... at this point, most changes are strictly QoL improvements or new-features. The latest commit on main is much more likely to be usable than the latest release, and we will continue to only provide support for the latest main.

Thus, I don't see the benefit of pushing users into using tagged releases.

I'll approve, let's give it a try.

Since this is so early in the setup process, I also think it's fine to just delete the flag completely (no deprecation warning), as long as we've scrubbed references to it throughout the docs/code. It'll just error immediately for anyone using the flag and then they can fix it.

Copy link
Contributor

@jerryz123 jerryz123 left a comment

Choose a reason for hiding this comment

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

Thanks for bringing this up @joey0320

@jerryz123 jerryz123 merged commit a3a2c19 into main Jan 22, 2024
8 checks passed
@jerryz123 jerryz123 deleted the remove-skip-validate branch January 22, 2024 07:32
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.

6 participants