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

misc: add openlane #246

Merged
merged 2 commits into from
Oct 25, 2022
Merged

misc: add openlane #246

merged 2 commits into from
Oct 25, 2022

Conversation

proppy
Copy link
Contributor

@proppy proppy commented Sep 27, 2022

Fixes #244

/cc @QuantamHD @mithro

Copy link
Contributor

@ajelinski ajelinski left a comment

Choose a reason for hiding this comment

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

Apart from needs I see such logs:

Did you see them, are those acceptable? Otherwise LGTM.

@proppy
Copy link
Contributor Author

proppy commented Oct 18, 2022

OpenLane UNKNOWN
(with mounted scripts from fatal: not a git repository: '$PREFIX/share/openlane/.git')

Maybe we could patch https://github.com/The-OpenROAD-Project/OpenLane/blob/32da932761213af689f10088d671e1e3dc38f273/flow.tcl#L395-L398 so that it include the conda version?

.../openlane/designs/inverter/runs/RUN_2022.09.27_15.06.12/results/signoff/inverter.klayout.gds' wasn't found. Skipping GDS XOR.

We should probably set RUN_KLAYOUT_XOR=0 in misc/openlane/disable-missing-tools.tcl so that it get skilled:
https://github.com/The-OpenROAD-Project/OpenLane/blob/32da932761213af689f10088d671e1e3dc38f273/flow.tcl#L151

@ajelinski
Copy link
Contributor

Maybe we could patch https://github.com/The-OpenROAD-Project/OpenLane/blob/32da932761213af689f10088d671e1e3dc38f273/flow.tcl#L395-L398 so that it include the conda version?

Perhaps adding echo $PKG_VERSION >$PREFIX/share/openlane/install/installed_version to build.sh could be enough thanks to: https://github.com/The-OpenROAD-Project/OpenLane/blob/32da932761213af689f10088d671e1e3dc38f273/flow.tcl#L381

@proppy
Copy link
Contributor Author

proppy commented Oct 18, 2022

Perhaps adding echo $PKG_VERSION >$PREFIX/share/openlane/install/installed_version to build.sh could be

Yes figured this out too with ac16102 but I think we also need The-OpenROAD-Project/OpenLane#1439.

@proppy
Copy link
Contributor Author

proppy commented Oct 18, 2022

@ajelinski what do you think of bundling tcllib like I'm doing in https://github.com/proppy/conda-eda/tree/master/misc/tcllib ? would you prefer to have this as a separate PR?

@ajelinski
Copy link
Contributor

@ajelinski what do you think of bundling tcllib like I'm doing in https://github.com/proppy/conda-eda/tree/master/misc/tcllib ? would you prefer to have this as a separate PR?

I have no problem with including it in this PR as such.

I don't know tcl very well, is tcllib OS-independent? Shouldn't it be actually built with conda-build?

@proppy
Copy link
Contributor Author

proppy commented Oct 18, 2022

I don't know tcl very well, is tcllib OS-independent? Shouldn't it be actually built with conda-build?

There is actually a conda-forge package we could backport: https://github.com/conda-forge/tcllib-feedstock

Looking at the underlying installer it seems that it's mostly copying files around: https://core.tcl-lang.org/tcllib/file?name=installer.tcl&ci=tip so I'm not sure why it's not noarch https://anaconda.org/conda-forge/tcllib/files

@ajelinski
Copy link
Contributor

There is actually a conda-forge package we could backport: https://github.com/conda-forge/tcllib-feedstock

Looking at the underlying installer it seems that it's mostly copying files around: https://core.tcl-lang.org/tcllib/file?name=installer.tcl&ci=tip so I'm not sure why it's not noarch https://anaconda.org/conda-forge/tcllib/files

I'll "build" and upload the tcllib package from a branch then and move the package on the litex-hub channel to main. IMO adding it to the repository to have it built nightly doesn't make sense in such a case especially that the source doesn't change at all.

Perhaps it isn't noarch because it has the tk package in run requirements? Not sure but maybe let's keep it as it is especially that we only need the package for Linux, WDYT?

@proppy
Copy link
Contributor Author

proppy commented Oct 19, 2022

I'll "build" and upload the tcllib package from a branch then and move the package on the litex-hub channel to main.

Oh I didn't realize you had upload write to anaconda main! I'll keep this in mind for other commonly used package that are only in conda-forge.

Perhaps it isn't noarch because it has the tk package in run requirements?

Ah that would make sense, looking at the various module documentation it seems that only only few package depends on Tk:
https://github.com/tcltk/tcllib/search?q=%22%5Brequire+Tk%22+titledesc:

  • map
  • comm
  • tepam
  • math

Looking at the installer it seems that there is a +excluded flag https://github.com/tcltk/tcllib/blob/efe4404da89b4bd7adafeadd4106a1a4592492ab/installer.tcl#L474 we could leverage to remove them (or we could patch out the part that depends on tk). I'm not familiar with the main requirement and that might be moot since tk looks already well packaged: https://anaconda.org/main/tk

@ajelinski
Copy link
Contributor

ajelinski commented Oct 19, 2022

Oh I didn't realize you had upload write to anaconda main! I'll keep this in mind for other commonly used package that are only in conda-forge.

Oh no, I'm not that powerful. :-) I only meant moving the package from the ci-<branch>-<run_id> to the main Conda label. It only makes the package built by CI from a conda-eda branch "visible" without providing the label name (because main is the default label).

Ah that would make sense, looking at the various module documentation it seems that only only few package depends on Tk: https://github.com/tcltk/tcllib/search?q=%22%5Brequire+Tk%22+titledesc:

* map

* comm

* tepam

* math

Looking at the installer it seems that there is a +excluded flag https://github.com/tcltk/tcllib/blob/efe4404da89b4bd7adafeadd4106a1a4592492ab/installer.tcl#L474 we could leverage to remove them (or we could patch out the part that depends on tk). I'm not familiar with the main requirement and that might be moot since tk looks already well packaged: https://anaconda.org/main/tk

Is having a noarch package worth such a struggle?

I've just made the tcllib package for Linux available on the LiteX-Hub channel so please test it and modify this package to use it.

@proppy
Copy link
Contributor Author

proppy commented Oct 19, 2022

Is having a noarch package worth such a struggle?

Not really, but it also allow us to remove the transitive dependencies on tk for openlane :)

@proppy
Copy link
Contributor Author

proppy commented Oct 19, 2022

I've just made the tcllib package for Linux available on the LiteX-Hub channel so please test it and modify this package to use it.

Done.

@proppy
Copy link
Contributor Author

proppy commented Oct 19, 2022

Not really, but it also allow us to remove the transitive dependencies on tk for openlane :)

Filed #250 (comment) so that we can explore this separatly.

@proppy
Copy link
Contributor Author

proppy commented Oct 20, 2022

squash the commits.

Done.

@proppy proppy requested a review from ajelinski October 20, 2022 01:30
@ajelinski
Copy link
Contributor

Unfortunately the build failed -- there's something wrong with the patch @proppy

Co-authored-by: Adam Jeliński <59468166+ajelinski@users.noreply.github.com>
@proppy
Copy link
Contributor Author

proppy commented Oct 20, 2022

Unfortunately the build failed -- there's something wrong with the patch @proppy

refreshed the patch

Co-authored-by: Adam Jeliński <59468166+ajelinski@users.noreply.github.com>
@ajelinski ajelinski merged commit eec67d8 into hdl:master Oct 25, 2022
@proppy
Copy link
Contributor Author

proppy commented Oct 25, 2022

@ajelinski curious why the CI still fails?

@ajelinski
Copy link
Contributor

The patch was merged upstream yesterday. I've just merged and removed the patch. :)

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.

add OpenLane package
2 participants