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

Add Travis CI config #28

Merged
merged 4 commits into from
Sep 5, 2017
Merged

Add Travis CI config #28

merged 4 commits into from
Sep 5, 2017

Conversation

WasabiFan
Copy link
Member

@WasabiFan WasabiFan commented Aug 27, 2017

https://travis-ci.org/WasabiFan/vscode-ev3dev-browser/builds/268983354

This was originally taken from here: https://code.visualstudio.com/docs/extensions/testing-extensions#_running-tests-automatically-on-travis-ci-build-machines

I modified it to specify the correct language and omitted running the tests given that we don't have any. It might be nice to get AppVeyor builds running but I can't remember their licensing model. #29

@dlech
Copy link
Member

dlech commented Aug 27, 2017

What does CI do for us? We have no tests (#19 ).

@WasabiFan
Copy link
Member Author

#27 (comment)

.travis.yml Outdated
node_js: lts/*

before_install:
- if [ $TRAVIS_OS_NAME == "linux" ]; then
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We probably don't need any of this.

We don't use any native modules, so CXX and CC are not needed.

I doubt need X either.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These were for running tests, which we aren't doing.

.travis.yml Outdated
fi

install:
- npm install
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs to be npm install --no-optional.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The point of optional dependencies is that they are optional... i.e., you don't need to specify that flag.

.travis.yml Outdated

os:
- osx
- linux
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just building on Linux should be enough. We don't have anything platform dependent.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"I don't expect anything to break on other platforms" is not the same as "building on other platforms is a detriment". We get it for no additional cost, so I don't see a reason to remove macOS builds.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it is responsible to use "free" resources when you don't really need them.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't "really need" CI builds as a whole, or GitHub collaboration, or any of the other services we use. We've decided to adopt them because they provide benefit to us and our project. The case of building on other platforms is no different. That most of our code isn't special-cased for certain platforms certainly doesn't mean that it won't have platform-specific problems.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are really wearing me out will all this debate about things that are really not that important in the whole scheme of things (e.g. life, the universe, and everything).

This is a really small project and I haven't felt a need for CI at all. But if you really feel it would be helpful, we can set up travis to do a quick build check on Linux. That is my offer. End of discussion.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are really wearing me out will all this debate about things that are really not that important in the whole scheme of things (e.g. life, the universe, and everything).

I think the debates over relatively small things are a result of the frequent criticism on subjects that are even less consequential. It's things like this that make someone resistant to productive discussion over points that are legitimately relevant. If you make it seem like you're needlessly criticizing someone else's work (whether or not you think it's with good reason), when you have a disagreement over something meaningful neither side is going to be interested in compromise. This is something I've heard voiced by multiple career managers: if you want to produce the best result both for users and your team, you must let the little debates slide so you can push back when you have a strong opinion on a meaningful topic. In the same way that you may think that continuous integration isn't important, I don't think that spaces after ifs or the type of quote I use is. Digging in on one side will inevitably be met with an equal reaction from the other, even if it isn't immediate.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I admit that I am totally guilty of exactly what I am complaining about many times over. And if I have offended because of this, I am sorry. Part of me would like to say more about the specifics of why so many nit picks and what I really meant (certainly not criticism) since that seems to have rubbed you the wrong way, but it seems hypocritical at this point.

So, please keep up the good work and I will do my best to be more mindful of how I come across in reviews.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

to be more mindful of how I come across in reviews.

...and to do a better job making it clear what is important to me and what is no big deal.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you. I will certainly try as best as I can from here forward to be receptive to the larger asks and evaluate whether/how there may be a solution that keeps both of us happy so we don't get into binary debates like this. On the topic of formatting and style, I haven't intentionally gone against the format you've laid out but at the same time I haven't spent much of any effort to adhere to it; I'll take a closer look at my code to make sure that I follow that so we aren't leaving ourselves with unspoken issues of that sort.

As for this PR, I'll take a look tomorrow and get it finished up; I plan to remove the macOS builds and keep it to just Linux until we find a need for other platforms. I believe that the OS differences could become significant for us in the future, but as you said it's a value judgment and right now it seems like the simple solution is enough for the vast majority of cases.

@dlech dlech mentioned this pull request Aug 27, 2017
@dlech dlech merged commit 7feefd5 into ev3dev:master Sep 5, 2017
@dlech
Copy link
Member

dlech commented Sep 5, 2017

Thanks.

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