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

changed LVS config from shell to json #204

Merged
merged 7 commits into from
Jun 13, 2023

Conversation

marwaneltoukhy
Copy link
Member

@marwaneltoukhy marwaneltoukhy commented Jun 11, 2023

@d-m-bailey Please take a look, and test it

json config file can be found here: efabless/caravel_user_project#274

@d-m-bailey
Copy link
Contributor

@marwaneltoukhy
A few questions and possible suggestions.

        env_var = w.split("$")[1]
        if env_var in lvs_env:
            if not is_path(value):
                value = value.replace(w, lvs_env.get(env_var))
            else:
                value = os.path.join(os.path.dirname(value), os.path.splitext(value)[0].replace(w, lvs_env.get(env_var)) + os.path.splitext(value)[1])
  1. Since w will always be $[A-Za-z_]*, I think env_var = w[1:] might be clearer than w.split("$")[1] although both give the same results. We're not allowing the shell syntax ${var}, correct?

  2. Why do you have different replacement logic based on whether value is a path or not?

  3. Using a global replace may cause problems for a strings like $PDK/$PDK_ROOT.

                value = value.replace(w, lvs_env.get(env_var))

I recommend limiting the replacement to the first occurrence.

                value = value.replace(w, lvs_env.get(env_var), 1)

It looks like this pull request is removing the option to not run LVS. Maybe not having a lvs_config file would be a sufficient reason not to run LVS and issue a warning? (is that what currently happens?)

@marwaneltoukhy
Copy link
Member Author

It looks like this pull request is removing the option to not run LVS. Maybe not having a lvs_config file would be a sufficient reason not to run LVS and issue a warning? (is that what currently happens?)

We are going to remove the check from the platform, but we agreed that the flag shouldn't be added here, and I created a target in the user project for users who don't want to run LVS as part of precheck. So basically the normal flow locally will run LVS automatically. we're requiring users to have the lvs config, if not the precheck will fail.

@marwaneltoukhy marwaneltoukhy merged commit c40bbdf into lvs_integration Jun 13, 2023
@marwaneltoukhy marwaneltoukhy deleted the json_lvs_config branch June 13, 2023 20:17
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