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

Tcl connector: implement API v1 #2335

Merged
merged 1 commit into from
Oct 19, 2022
Merged

Conversation

DigitalBrains1
Copy link
Member

@DigitalBrains1 DigitalBrains1 commented Sep 30, 2022

A breaking change was made to the API. I realized the extra indirection
of defining a procedure was not actually needed in order to elegantly
pass in the argument specifying where to create the namespace.

Vivado tests in clash-testsuite now use the Tcl connector, and handle
a top component name that differs from the Haskell function that
specifies that component. Closes: #2264

Still TODO:

  • Write a changelog entry (see changelog/README.md)
  • Check copyright notices are up to date in edited files

@DigitalBrains1
Copy link
Member Author

DigitalBrains1 commented Sep 30, 2022

You could leave this PR with a partial implementation open until you really need it, and perhaps another PR with the full implementation will make this obsolete before that. It depends on how quickly you'll get to the situation where you need to pass Xilinx.Floating to Vivado.

@DigitalBrains1 DigitalBrains1 force-pushed the tcl-connector-partial branch 5 times, most recently from e1db215 to 0d85d82 Compare October 7, 2022 14:38
@DigitalBrains1 DigitalBrains1 changed the title Tcl connector: partially implement Clash<->Tcl API Tcl connector: implement API v1 Oct 11, 2022
@DigitalBrains1
Copy link
Member Author

I've just pushed the full Tcl connector implementation instead of the earlier partial one.

@DigitalBrains1
Copy link
Member Author

@mergify rebase

@mergify
Copy link

mergify bot commented Oct 15, 2022

rebase

✅ Branch has been successfully rebased

Copy link
Member

@martijnbastiaan martijnbastiaan left a comment

Choose a reason for hiding this comment

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

LGTM! I'm not familiar enough with TCL to catch subtle mistakes, but given that the testsuite uses it now I don't see a reason to not merge.

@DigitalBrains1
Copy link
Member Author

Ah, Martijn, I hadn't expected you around. If you're still following this, I'd like to make some comments. I think this is mergeable, but I noted two things:

  1. I looked at PR GenTcl restructuring #2319, specifically Clash.LocatedManifest. I intended to include it some way here, but it ran afoul of issue Refactor BuildSpecific to pass in the full qualified name #2264. PR 2319 looked up the manifest with manifestLocationFromStrName hdlDir qualName, but the code here that fixes 2264 is:
    do
    [...]
    manifests <- getManifests (dir </> "*" </> manifestFilename)
    let [(dropFileName -> topEntityDir, _)] =
            filter ((== top) . topComponent . snd) manifests
    I couldn't really see how you would want to use Clash.LocatedManifest here, what modifications to that module would work for your purposes.
  2. You noted that Clash.DataFiles.tclConnector returning a path to a Cabal data file might not work under all circumstances with Stack, and I think that is still the case: discussions pointed to Add stack build --prefix option commercialhaskell/stack#848 , which is still open. Generally the suggestion is to use file-embed (which isn't perfect in Stack either: stack ghci can't find file-embed files when the package is in a subdirectory commercialhaskell/stack#1057 ). Should I, already at this stage, re-implement Clash.DataFiles.tclConnector to return a ByteString with the contents rather than a path to the file on the filesystem? Or shall we do this in a later PR?

@vmchale
Copy link
Contributor

vmchale commented Oct 17, 2022

Should I, already at this stage, re-implement Clash.DataFiles.tclConnector to return a ByteString with the contents rather than a path to the file on the filesystem? Or shall we do this in a later PR?

I prefer the data-files approach - but in any case I think you don't need to use file-embed to get this specific PR merged in!

Copy link
Contributor

@vmchale vmchale left a comment

Choose a reason for hiding this comment

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

I am approving; the test suite still works on my computer.

(The TCL looks strictly better as far as I can tell)

A breaking change was made to the API. I realized the extra indirection
of defining a procedure was not actually needed in order to elegantly
pass in the argument specifying where to create the namespace.

Vivado tests in `clash-testsuite` now use the Tcl connector, and handle
a top component name that differs from the Haskell function that
specifies that component. Closes: #2264
@DigitalBrains1 DigitalBrains1 merged commit a5bbff7 into master Oct 19, 2022
@DigitalBrains1 DigitalBrains1 deleted the tcl-connector-partial branch October 19, 2022 13:46
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.

Refactor BuildSpecific to pass in the full qualified name
3 participants