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

Small additions to the os condition #922

Merged
merged 10 commits into from
Jul 1, 2018
Merged

Small additions to the os condition #922

merged 10 commits into from
Jul 1, 2018

Conversation

siad007
Copy link
Member

@siad007 siad007 commented May 22, 2018

No description provided.

@siad007 siad007 added this to the 3.0.0-alpha2 milestone May 22, 2018
@codecov
Copy link

codecov bot commented May 22, 2018

Codecov Report

Merging #922 into master will not change coverage.
The diff coverage is 81.81%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #922   +/-   ##
=========================================
  Coverage     44.86%   44.86%           
  Complexity     9875     9875           
=========================================
  Files           499      499           
  Lines         23934    23934           
=========================================
  Hits          10737    10737           
  Misses        13197    13197
Impacted Files Coverage Δ Complexity Δ
...asses/phing/tasks/system/condition/OsCondition.php 85.71% <81.81%> (ø) 13 <0> (ø) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 752b178...4ccae73. Read the comment docs.

@kenguest
Copy link
Member

Is netware still "a thing"? I'm not sure about the need to add support for operating systems that have been discontinued.

@siad007
Copy link
Member Author

siad007 commented May 22, 2018

But this is a addition to the OsCondition so why should this condition not be able to determine a netware system?

@kenguest
Copy link
Member

kenguest commented May 24, 2018

A quick search for "PHP on NetWare" shows that "PHP for NetWare® is based on open source PHP version 4.2.3 and PHP version 5.0.5.".

Unless that is grossly inaccurate and PHP 7.1 can run on that discontinued operating system, I think you are adding code that will never execute.

(https://github.com/phingofficial/phing/blob/master/composer.json currently specifies a minimum version of PHP 7.1 for phing, but you know that anyway)

@mrook
Copy link
Member

mrook commented May 24, 2018

I have the same comments about "dos". So, let's stick to detecting linux/mac/windows?

@siad007 siad007 changed the title Additional support for dos, mac and netware. Small additions to the os condition Jun 2, 2018
@siad007
Copy link
Member Author

siad007 commented Jun 2, 2018

Hmm there seems to be a problem with a test from #907 on https://travis-ci.org/phingofficial/phing/jobs/387075667 - strange that this test was not failing before?! Any hints?

@mrook
Copy link
Member

mrook commented Jun 3, 2018

@siad007 it did actually fail before, but the failure was ignored for a while because of the codecov upload. See 8aea675#diff-b4553af39acae935d505c76e139d4aa3.

@siad007
Copy link
Member Author

siad007 commented Jun 3, 2018

@mrook it becomes stranger after executing the failing test on my environment (windows) with no failure.

@mrook
Copy link
Member

mrook commented Jun 4, 2018

@siad007 yeah I've noticed the test runs fine when run on its own, but fails when the entire test suite is run. Side-effect of another test? Cleanup gone wrong?

@siad007 siad007 merged commit fd10c98 into master Jul 1, 2018
@siad007 siad007 deleted the siad007-patch-1 branch July 1, 2018 20:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

3 participants