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

Make OSL & OSO parsers re-entrant. #969

Merged
merged 7 commits into from
Oct 6, 2020

Conversation

marsupial
Copy link
Contributor

Description

Probably a bit premature, but re-basing showed some conflicts with recent changes and wanted to make sure toes aren't being stepped on.

Also wanted to get a sense from CI if this affects normal usage compiling much.

Tests

Forthcoming
Test-suite is working, but could use an additional test of multiple threads / compilers.

Checklist:

  • I have read the contribution guidelines.
  • I have previously submitted a Contributor License Agreement.
  • I have updated the documentation, if applicable.
  • I have ensured that the change is tested somewhere in the testsuite (adding new test cases if necessary).
  • My code follows the prevailing code style of this project.

@lgritz
Copy link
Collaborator

lgritz commented Jan 15, 2019

I'm really liking the look of this!

I'm embarrassed that so much of AST was still referencing through the oslcompiler pointer instead of the local m_compiler that was already in the node. I'm guessing that m_compiler was added late in the development of the oslc guts and I didn't go back and fix all the pointers to stop using the global one.

Great bison work, you certainly know some fancy tricks I wasn't aware of.

There's a lot to go over in detail, I'll wait for you to iron out the obvious CI failures and whatnot, then let me know when it's ready for a more thorough review. But this is awesome.

If you're really adventurous, there is probably an analogous set of changes to the OSOReader and osogram.y that would let us concurrently load compiled shaders at render time as well.

@marsupial
Copy link
Contributor Author

Hopefully CI will pass fully.
Still the issue of TypeSpec lurking.

@marsupial marsupial force-pushed the PR/OSLCompiler branch 3 times, most recently from df3f58a to ab7c05b Compare January 16, 2019 19:49
@lgritz
Copy link
Collaborator

lgritz commented Sep 29, 2020

This seems to have slipped through the cracks. I think I was waiting for you to continue making changes to address failed CI, etc, and maybe never realized that things had settled down (did they?). I like the idea of this, if you want to rebase on top of the current master and see if we can pass CI, we can definitely revive this.

@marsupial marsupial changed the title Initial attempt to allow multiple OSLCompiler instances. Make OSL & OSO parsers re-entrant. Oct 1, 2020
@marsupial
Copy link
Contributor Author

Updated to remove reliance on statics when parsing oso and osl files.

Rebasing proved difficult, so dropped the more exhaustive changes to allow multiple OSLCompiler instances. (IIRC there was only one remaining outstanding issue of how to deal with the global struct-table.)

Linux/Mac also make the bison/yacc symbols hidden, to further make sure two libraries can't interfere.

@lgritz
Copy link
Collaborator

lgritz commented Oct 3, 2020

I like this but I'm having trouble not breaking people's builds on Mac where, as you noted, you MUST use the Homebrew flex and bison, which are newer than the versions that come with xcode and are in /usr/bin (at least on my version of MacOS and Xcode). This can be fixed with the combination of installing those packages on homebrew and also making sure FLEX_ROOT and BISON_ROOT point there. But that's brittle in the sense that most users will not know to do this until after they've been struggling with broken builds.

I don't mind reliance on the newer version, but right now I'm trying to tinker with (a) figuring out what the minimum version actually is so that we can enforce it, and (b) making the cmake search on Mac check the homebrew area first automatically.

When you made the PR, did you have it set up so that I can push additional fixes on to your PR? If so, I will try to fix this and amend your PR. If not, I'll stash it someplace and point you to it to integrate the changes yourself. Stay tuned.

@lgritz
Copy link
Collaborator

lgritz commented Oct 3, 2020

Yeah, I don't seem to have push permissions on your repo. I'll have to stash this stuff in mine and then you can get it from there.

@lgritz
Copy link
Collaborator

lgritz commented Oct 3, 2020

@marsupial, do you see on this PR a checkbox for "Allow edits and access to secrets by maintainers"?

@lgritz
Copy link
Collaborator

lgritz commented Oct 4, 2020

@marsupial check out lgritz/OpenShadingLanguage branch PR/OSLCompiler where I've added 3 more commits that fix some minor things. If those look ok to you, then please incorporate them into this PR and then I'll merge it all.

rzulak and others added 7 commits October 5, 2020 16:00
Add logic to flexbison.cmake to prefer the Homebrew version if it's
found (and if no BISON_ROOT is set) because the system install in
/usr/bin is too old to properly build with the new reentrant parser
directives.

Signed-off-by: Larry Gritz <[email protected]>
@marsupial
Copy link
Contributor Author

Box was checked, but maybe github can't manage the permissions from 3 repos?
Either way, pushed your commits on top

@lgritz
Copy link
Collaborator

lgritz commented Oct 5, 2020

Great, I'll merge when the CI tests are done.

@lgritz lgritz merged commit a3d8ebf into AcademySoftwareFoundation:master Oct 6, 2020
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