Closed ginty closed 3 years ago
Not sure what is wrong with the tests, some mailer test failing - it was all fine on this branch till I merged in master, can you take a quick look please @coreyeng ?
I added some new files and based on this my guess is that it didn't get moved to the right place.
As for Maturin, the reason I didn't use it for origen is that I want to wrap up the CLI, python package, and origen.py/.so all in a single install package. I don't think Maturin can do this. Or, at least I didn't see how to do it back in June. I also think Maturin just builds a single package, not a 'python version specific' package, which I think is safer as PyO3 may, at times, have different paths depending on Python version, and we build one for each. During the regressions, we build with the Python version from scratch, so we don't actually test that a generic library will work across Python 3.6 - 3.8.
But, this may be based on older Maturin knowledge as well.
Ah, I see, you ran Maturin on a matrix of the Python versions and OS. I think I debugged just a single one and it was easy enough to extend from there. But I'm still unsure if Maturin can wrap the CLI up with the package nicely. I wanted to avoid requiring a separate CLI download, or from the package trying to download anything behind-the-scenes (like perforce
did back in O1 a few years ago)
I think Origen Metal tests should be part of the main action. We can provide options to only run certain builds for debug, but as a default I think the entire project's regressions should always run, especially for any PR.
For publishing origen, I think something needs to be added somewhere to replace the path links to origen_metal with a reference to a released version number.
Yeah, I ran into something similar with the Rust crate. Poetry may have something to help. I'll look into it when I get to releasing a packaged with metal as a dependency.
Also looks like the only features that really got pulled so far are the logger and file utils? Once merged, I can try and switch to those.
Yeah, not sure if Maturin can handle bins or not, I think it seems to provide options to set most things in the package metadata and it does seem to package whatever you drop into the python dir.
OK I'll add the tests soon.
Yeah I haven't moved much into metal yet, was planning just to transfer things bit by bit as I need them. Most of the utilities should be easy to move, then the other thing I will want soon is some of the compiler infrastructure (like the node and processor foundations) so that we can add a PDL parser to metal, though that might be a bit more tricky to de-couple.
Hi @coreyeng, as discussed, this adds an initial drop of Origen Metal, Origen without the app.
I've moved a few things out of Origen (some of the file utils and the differ), just to check that everything is hooked up and can compile correctly.
A few things to note:
anyhow::Result
rather thanorigen::Result
. The former seems to be the best practice in the community now and its actually a drop-in replacement for our equivalent. It has some advantage in that you can append context every time you just pass it up the tree with?
, which generates more meaningful error messages.python/
topython/origen/
, so we can putpython/origen_metal
alongside itpython/origen_metal
for launching a console session, but its not called by the CLI yetimport origen_metal._origen_metal
, which I think is different to the main origen where it is justimport _origen
. I've used maturin to build the python package and I just couldn't get it to work the other way. However, this seems like it will not be a big deal and creating the build was actually pretty trivial using maturin. It's came on quite a bit since I last looked at it and I would recommend that you re-consider it for origen. One of the main advantages is that you can easily build for manylinux which means there will be no issues importing the extenion on any Linux box.Where do you want me to add the tests, as part of the main regression Action or a standalone one?
For publishing origen, I think something needs to be added somewhere to replace the path links to origen_metal with a reference to a released version number.