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

Gh actions - first pass #750

Closed
wants to merge 38 commits into from
Closed

Gh actions - first pass #750

wants to merge 38 commits into from

Conversation

nerdvegas
Copy link
Contributor

Notes:

  • Windows is not passing, help appreciated! (Cmake related?)
  • Badges don't work, I don't know why.

A couple of test-related things were changed:

  • There was a bug causing only the first shell to be tested with funcs decorated in per_available_shell.
  • Verbosity dropped on solver tests, was making it pretty hard to go through the Actions output.
  • The "logging" test was removed. Failed logging won't actually raise an exception, see https://docs.python.org/2/howto/logging.html#exceptions-raised-during-logging. The existing test was actually failing and putting a lot of noise into the test output. Given the simplicity of the test, and the fact that exceptions won't get raised, I thought it best just to remove it.
  • We were getting a bug where "decode" was called on the stdout arg from a subprocess.Popen().communicate() call. This is actually an issue with the powershell plugin. It was using universal_newlines=True, which changes the return type; see https://docs.python.org/3.4/library/subprocess.html#frequently-used-arguments. I am not clear on why this was added so I'm unsure of side effects. However it does seem as though if this is necessary, it could be passed through in **Popen_args instead anyway.

One thing we should definitely do is port to pytest and do away with the per_available_shell silliness. I'd be interested in adding a --test option to install.py, which would then install the rez-selftest tool, and pytest (as a native requirement in the installation venv, not a vendored lib). I think it'd be better to not install rez-selftest if --test wasn't specified, since it would require pytest in order to work.

Another thing we should do is update the rez-selftest -s arg to take multiple shells, and for each of those, to assert that the shell is available, rather than skip over it if not. Right now it isn't that clear whether a given shell is actually getting tested in the CI; for eg, rez could think the shell is unavailable (which in itself may be a bug). So the way I'd see it working is, we would do the apt-get install etc of the shells we want to test, then pass those shells to rez-selftest also, which would fail if any of those shells are not available / rez doesn't know about them.

ajohns added 25 commits September 26, 2019 20:31
-removed travis
-logging handlers don't actually raies exceptions, see https://docs.python.org/2/howto/logging.html#exceptions-raised-during-logging
-...we could get them to, but at this point I think it's too much complexity for a trivial case
-this causes return values to be strs rather than byte streams
-this was causing the decode('utf-8') error, not py3.6/7 as previously thought
@KelSolaar
Copy link
Contributor

KelSolaar commented Sep 27, 2019

Badges don't work, I don't know why.

They will once you merge the changes into master :) Would be also arguably quite traumatic for them to update each time a PR is opened ;)

@KelSolaar
Copy link
Contributor

Btw are you planning to squash the history?

@nerdvegas
Copy link
Contributor Author

nerdvegas commented Sep 27, 2019 via email

@KelSolaar
Copy link
Contributor

History - you mean the run history..? Not even sure if that's possible yet.

Nah, the commit one, it scorched my retina! :)

image

@nerdvegas
Copy link
Contributor Author

nerdvegas commented Sep 27, 2019 via email

@KelSolaar
Copy link
Contributor

Sure absolutely make sense, something I noticed before but they fixed during the last few days is that file parsing errors were creating junk on the left actions sidebar. It is gone now, for the better :)

@nerdvegas
Copy link
Contributor Author

nerdvegas commented Sep 27, 2019 via email

@KelSolaar
Copy link
Contributor

I'm a bit surprised the workflow language is so
limited though

It is almost exactly the Azure Pipelines stuff, and yeah there are some very frustrating bits! It will get better though, can see things changing on a per-day basis almost!

@JeanChristopheMorinPerso
Copy link
Member

I'm wondering, should we test on CentOS instead of Ubuntu as CentOS is probably way more used than Ubuntu in VFX?

@JeanChristopheMorinPerso
Copy link
Member

Well screw my last comment, they don't provide anything other than Ubuntu :( .

@bfloch
Copy link
Contributor

bfloch commented Sep 27, 2019

Great work!

Windows failing is expected.
It is being addressed in #737 and #703. Waiting for feedback.

We could skip the cmake test on windows for now - I don't believe rez has a strict dependency on cmake. Everything else should pass though.

@bfloch
Copy link
Contributor

bfloch commented Sep 27, 2019

windows.yaml
# TODO install shells (which ones need installing?)

PowerShell (5.x) and cmd should be build in Windows. We just need pwsh (PowerShellCore 6.x):
https://github.com/PowerShell/PowerShell/releases

EDIT: Better docs https://docs.microsoft.com/en-us/powershell/scripting/install/installing-powershell-core-on-windows?view=powershell-6

@bfloch
Copy link
Contributor

bfloch commented Sep 27, 2019

Worth checking if pwsh is not installed by default on all platforms:
https://help.github.com/en/articles/workflow-syntax-for-github-actions#using-a-specific-shell

If not something like might do the trick (just tested locally):

steps:
  - name: Install pwsh
    shell: PowerShell
    run: |
      [Net.ServicePointManager]::SecurityProtocol = [Net.SecurityProtocolType]::Tls12
      Invoke-WebRequest http://github.com/PowerShell/PowerShell/releases/download/v6.2.3/PowerShell-6.2.3-win-x64.msi -OutFile PowerShell-6.2.3-win-x64.msi
      Start-Process -Wait -FilePath msiexec -ArgumentList /package, PowerShell-6.2.3-win-x64.msi, /quiet, ADD_EXPLORER_CONTEXT_MENU_OPENPOWERSHELL=1, ENABLE_PSREMOTING=1, REGISTER_MANIFEST=1

@nerdvegas
Copy link
Contributor Author

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.

4 participants