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

Gojsonnet #728

Closed
wants to merge 10 commits into from
Closed

Gojsonnet #728

wants to merge 10 commits into from

Conversation

pvanderlinden
Copy link
Contributor

Fixes issue #144
Might also solve #704

Proposed Changes

  • optional use of gojsonnet
  • use spawn method instead of fork for multiprocessing (as gojsonnet will not work)
  • copy cached as a parameter. I have not looked into what it is actually used for, but this will make sure using spawn will actually work.

@ademariag
Copy link
Contributor

Thank you @pvanderlinden. Join our #kapitan channel on the kubenetes slack if you haven't already.

@ademariag ademariag requested a review from ramaro April 2, 2021 11:47
@pvanderlinden
Copy link
Contributor Author

Mmh, changing to spawn causes issue with pyinstaller.
It seems like newer versions of pyinstaller solve these issues. Unfortunately the newer version of pyinstaller doesn't work with staticx. The issue has been resolved, but no new release has been yet.
See:
JonathonReinhart/staticx#71
pyinstaller/pyinstaller#5511

@ramaro
Copy link
Member

ramaro commented Apr 27, 2021

Hey @pvanderlinden these look great and work great on macOS! I'm gonna give it a try on Linux just to validate things but looking great so far - thank you 😄

@ramaro
Copy link
Member

ramaro commented Apr 27, 2021

We can also now remove this block https://github.com/kapicorp/kapitan/blob/master/kapitan/cli.py#L99

@pvanderlinden
Copy link
Contributor Author

It looks like pyinstaller has a new release which should fix the issue using spawn and building a binary. I should have time to remove the code you linked and test that 🤞

@pvanderlinden
Copy link
Contributor Author

I'm having troubles getting the tests for the binary up and running, even from the master branch. So no idea if my local fix actually fixes anything

@pvanderlinden
Copy link
Contributor Author

@ramaro I'm trying to fix this PR, I might have a solution, but I can't test it. The issue is that I can't get the binary test to work at all, not even on master. I tried to contact you on slack twice in the last month, but unfortunately no response. Any idea how to get this PR moving?

@ramaro
Copy link
Member

ramaro commented Jun 16, 2021

Hey @pvanderlinden apologies, I often miss slack notifications! As previously discussed in a slack thread that I didn't miss, let's discontinue the binary - it's not worth it. I created #740 for this and will be pushing it to master soon. Hopefully this will unblock the Gojsonnet work :) Thanks for the patience.

@ramaro
Copy link
Member

ramaro commented Jun 16, 2021

Removal PR at #741

@pvanderlinden
Copy link
Contributor Author

THanks Ramaro, I have subscribed to the other PR, and will revisit this PR after it's merged

@janeklb
Copy link
Contributor

janeklb commented Jul 11, 2021

Hi @pvanderlinden, just in case you've missed it #741 was merged a couple of weeks ago.

If you're too busy I'd be more than happy to continue where you (and @ramaro!) left off with this work. Looking forward to having this in the main trunk!

@janeklb janeklb mentioned this pull request Jul 11, 2021
@janeklb
Copy link
Contributor

janeklb commented Jul 11, 2021

I hope you don't mind - I have opened up a new PR using all of your work but based on master's HEAD, just to see if travis-ci tests pass: #753. Will close it once we have the data. (Update: unfortunately seems like travic-ci integration is broken atm)

(have also removed the duplicate 'add cache' & 'enable cache' commits since they were just "merge artefacts", and removed the last two commits which, I assume, were meant to address the binary build failures).

The performance improvement is fantastic.

Also, worth noting that @pvanderlinden's branch fixes an issue that I was seeing @ramaro's original gojsonnet branch.

Fatal Python error: deallocating None
Python runtime state: finalizing (tstate=0x7fab37405b30)

Current thread 0x00000001134a1e00 (most recent call first):
<no Python frame>
[2]    54358 abort      kapitan compile --verbose

The compilation output (that I was seeing) was correct; however, the message is a worrying one.

@pvanderlinden
Copy link
Contributor Author

@janeklb I have been busy so far, I was planning to pick this up soonish. Thanks for rebasing, I will close this one.

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.

None yet

4 participants