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

Config finder make target #1328

Merged
merged 4 commits into from
Mar 6, 2023
Merged

Config finder make target #1328

merged 4 commits into from
Mar 6, 2023

Conversation

abejgonzalez
Copy link
Contributor

@abejgonzalez abejgonzalez commented Feb 6, 2023

Adds a new target to help users find some config fragments (also adds to the docs on how to use it).

Unfortunately, I wanted to do this through Scala reflection, however, that isn't possible since you can only look at the subclasses of sealed traits/classes. This is a simple solution that covers large majority of configs (assuming config. fragments are defined on a single line - "class NAME extends CONFIG\n"). This doesn't cover configs that don't inherit directly from Config... I don't have time to do that right now (or figure out a better way).

TODO:

  • Right now just finds configs that are on the same line (class NAME extends Config\n). Extend this to match on multi-line (is there a scala-parser in Python that we can just read the scala file and see if there is an object that matches this).
  • Do successive greps through the code base to find child classes that extend from the main class (stop after a certain depth)

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?

@abejgonzalez abejgonzalez self-assigned this Feb 6, 2023
@abejgonzalez
Copy link
Contributor Author

Opening this up to see if people have any better ideas on how to do this (without some of the flaws)

@jerryz123
Copy link
Contributor

Neat. Can you add this to build setup? I think this along with ctags can be the same step.

@abejgonzalez
Copy link
Contributor Author

abejgonzalez commented Feb 6, 2023

Neat. Can you add this to build setup? I think this along with ctags can be the same step.

This dumps the configs at will, not into some file... Did you want this to be more visible (hence printing at the start of build-setup)?

@jerryz123
Copy link
Contributor

Hmmm good point. Printing into a file probably isn't great for discoverability.

This should be added to CI somewhere though.

Copy link
Member

@sagark sagark left a comment

Choose a reason for hiding this comment

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

LGTM

@abejgonzalez abejgonzalez merged commit f7a39f8 into main Mar 6, 2023
joonho3020 pushed a commit that referenced this pull request Mar 7, 2023
* Add config-finder make target

* Add recursive functionality

* Add config finder to CI

* Workaround bash argument limit failures
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.

Somehow my review comment didn't get posted.

Can you fix the dump into /tmp? That's a bit concerning to me.

CONFIG_FRAG_LEVELS ?= 3
.PHONY: find-config-fragments
find-config-fragments: $(SCALA_SOURCES)
rm -rf /tmp/scala_files.f
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm afraid of this causing problems for shared installations. Can you make this dump the file within the repo?

sriramsridhar23 pushed a commit that referenced this pull request Mar 15, 2023
* Add config-finder make target

* Add recursive functionality

* Add config finder to CI

* Workaround bash argument limit failures
@abejgonzalez abejgonzalez deleted the config-finder branch April 20, 2023 20:37
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