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

.travis.yml is used incorrectly #4

Closed
annevk opened this issue Jan 14, 2017 · 18 comments
Closed

.travis.yml is used incorrectly #4

annevk opened this issue Jan 14, 2017 · 18 comments

Comments

@annevk
Copy link
Member

annevk commented Jan 14, 2017

When I run travis lint .travis.yml on e.g., URL's or DOM's resource, I get this:

[x] value for env section is empty, dropping
[x] value for addons section is empty, dropping
[x] in env section: value for global section is empty, dropping
[x] in env.global section: unexpected pair
[x] in addons section: unexpected key apt, dropping

This might explain why @sideshowbarker had to define the Java version in the script invocation. If the global is also not used, I guess the ENCRYPTION_LABEL thing we commit everywhere is useless too?

@annevk
Copy link
Member Author

annevk commented Jan 14, 2017

ENCRYPTION_LABEL is definitely needed per my attempts in whatwg/url#202 to figure out what's going on.

@annevk
Copy link
Member Author

annevk commented Jan 14, 2017

So it seems the syntax is somewhat incorrect. Turning it into

env:
  - ENCRYPTION_LABEL="82f6fcb5a224"

validates. (So dropping "global:" and replacing the colon with an equal sign.)

@annevk
Copy link
Member Author

annevk commented Jan 14, 2017

I suspect the issue with the Java stuff is that it only works if language is set to java, but that probably causes other things to fail? See https://docs.travis-ci.com/user/languages/java/.

@sideshowbarker
Copy link
Contributor

I suspect the issue with the Java stuff is that it only works if language is set to java, but that probably causes other things to fail?

Right. (Well that and also the fact that when I was testing it there was a Travis bug that’s now been fixed that was preventing it from working the way it should be. That said, I think the only effect of that being fixed is that we no longer need to specify the full path to the java binary. But there’s also no downside to specifying the full path to java the way we are now, so I think there’s no need to change it as this point.)

@domenic
Copy link
Member

domenic commented Jan 14, 2017

I'm suspicious of this travis lint tool; it seems pretty bogus. Some of the examples it rejects are copied directly from the Travis documentation.

I think we should not specify the full path to java if possible, as that will make us robust against Travis upgrades that change where the default Java is installed.

@sideshowbarker
Copy link
Contributor

I think we should not specify the full path to java if possible, as that will make us robust against Travis upgrades that change where the default Java is installed.

OK I can go back and change it

(But that said, I think it’s extremely unlikely that the path will ever change—because it’s not controlled by Travis but instead by the OS, Ubuntu. I think what’s more likely to happen is that Travis bug which made it necessary to begin will regress, or Travis could introduce some other bug and it would be necessary again while they’re fixing that.)

@domenic
Copy link
Member

domenic commented Jan 14, 2017

I didn't mean we should necessarily change it right now :). Maybe just if we're in the area...

@domenic
Copy link
Member

domenic commented Jan 23, 2017

FWIW I tried really hard to get Java-without-a-fully-qualified path to work over in whatwg/url#205 and Travis just wouldn't cooperate. So we can stay with the full path. It just means we'll need to skip running the checker when doing a "local deploy".

@sideshowbarker
Copy link
Contributor

FWIW I tried really hard to get Java-without-a-fully-qualified path to work over in whatwg/url#205 and Travis just wouldn't cooperate.

OK yeah I don’t understand either what the issue is but at some point soon I’ll make time at it myself again too and see if figure out what the best-practice way of doing it is—given we need to force use of Java8 rather than Java7 (which is still the default even in Trusty as far as I know).

@annevk
Copy link
Member Author

annevk commented Jan 24, 2017

For local deploy we might actually want to use the network API for the HTML checker. I wouldn't want to add a Java dependency locally.

@domenic
Copy link
Member

domenic commented Jan 24, 2017

Right now I have it just skipping the checker but network API sounds pretty reasonable. In fact it sounds pretty reasonable all the time; why did we decide on downloading the jar and executing locally?

@sideshowbarker
Copy link
Contributor

why did we decide on downloading the jar and executing locally?

because with the network API it’s only capable for reporting the filenames—basenames—for files it checks, rather that the full pathnames. So if we have, say, an index.html is top-level directory and another index.html in subdirectory, and the checker reports it found errors in the index.html file, then we don’t know which index.html the error is actually in.

But if we can live with that ambiguity, we could switch back to just using the network API to the checker.

@annevk
Copy link
Member Author

annevk commented Jan 24, 2017

So the network API does accept multiple files being uploaded, but then doesn't do relative reporting? Isn't that something that could be fixed somehow?

domenic added a commit to whatwg/dom that referenced this issue Jan 24, 2017
- Updates to the deduplicated deploy script; part of whatwg/meta#6
- As such, stops deploying commit snapshots for branches; part of whatwg/meta#9
- Fixes .travis.yml syntax; part of whatwg/meta#4
- Adds .gitattributes and .editorconfig; part of whatwg/meta#7
domenic added a commit to whatwg/dom that referenced this issue Jan 24, 2017
- Updates to the deduplicated deploy script; part of whatwg/meta#6
- As such, stops deploying commit snapshots for branches; part of whatwg/meta#9
- Fixes .travis.yml syntax; part of whatwg/meta#4
- Adds .gitattributes and .editorconfig; part of whatwg/meta#7
@sideshowbarker
Copy link
Contributor

So the network API does accept multiple files being uploaded, but then doesn't do relative reporting?

It doesn’t accept multiple files being uploaded in the same request. The network API only handles one doc per request (the entire servlet backend is built around handling only one doc per request).

Isn't that something that could be fixed somehow?

Sure it could be. But practically speaking it needs to be done outside the checker network API, in some (small) shell-script wrapper to just echo the full pathname before it sends each request.

Otherwise offhand the only way I can think of to implement it in the network API itself is to create a new query param called fullpathname or whatever to add to the POST URL. Which I’d rather not.


Details: The normal/preferred way to use the network API is to send the HTML document as the entity body of the POST. But the results in the response from that do not include the filename at all, so it’s not suitable when batch-checking multiple files (unless as I said above, you wrap it in some shell script that echoes the full pathnames).

It’s possible to get the filenames themselves (but not the pathnames) by using the network API to emulate a multipart/form-data file upload, like this:

curl -F out=gnu -F charset=foo -F file=@site/about.html https://checker.html5.org/

But in that case, even with a path (site/about.html) specified for the file param, the results only report the filename (minus any directory names):

"about.html": info warning: Overriding document character encoding from none to “foo”.
"about.html": error: Unsupported character encoding name: “foo”. Will sniff.

@annevk
Copy link
Member Author

annevk commented Jan 25, 2017

So currently we download the the whole HTML checker which is 25093868 bytes = ~24MiB. Using the network API instead is probably more reasonable for most of our setups.

@sideshowbarker
Copy link
Contributor

So currently we download the the whole HTML checker which is 25093868 bytes = ~24MiB. Using the network API instead is probably more reasonable for most of our setups.

Yeah, for specs with just one file to check—e.g., the DOM spec—I think it’s more reasonable both in not requiring a ~24MiB download each time and in being a couple of seconds faster in practice.

It will be faster because Travis has a pretty fast network connection—so the request and response happen very quickly—and because the jar comes with a hard ~2-second startup time (to start java and load the schemas, etc.). That 2 seconds is relatively expensive for checking just one file.

So yeah I reckon for most repos we should definitely change it to use the network API, and I’ll write up a snippet we can use for that asap.

In hindsight I realize now it was a bit of a distraction to start with setting this up for the HTML spec, because I ended up optimizing for that case, which is really a radically different case from the rest.

For the HTML spec, using the network API will be much lot slower—because for HTML it needs to send the ~8MB output/index.html single file for checking, then all 24 files in output/multipage for checking, then 15 files in output/demos.

Checking all those HTML spec output files using the jar locally takes about 8.5 seconds.

Checking them all using the network API takes 2 minutes and 30 seconds.

And all together those individual HTML files add up to ~21.5MiB to upload over the network.

Given all that, it seems like there’s no win to using the network API for checking the HTML spec files, but there is for pretty much all the other specs. So shall we change all the rest but keep the jar-based checking for the HTML spec?

@sideshowbarker
Copy link
Contributor

OK I think the following is gonna be the least-ugly way to do it in Travis with the network API:

(find . -name "*.html" -exec bash -c 'echo "Checking {}..."; \
  curl -s -H "Content-Type: text/html; charset=utf-8" \
  --data-binary @{} https://checker.html5.org/?out=gnu \
  | tee -a OUTPUT; echo' \;); if [ -s OUTPUT ]; then exit 1; fi

annevk added a commit to whatwg/fetch that referenced this issue Feb 2, 2017
- Updates to the deduplicated deploy script; part of whatwg/meta#11
- As such, stops deploying commit snapshots for branches
- Fixes .travis.yml syntax; part of whatwg/meta#4
- Adds .gitattributes and .editorconfig; part of whatwg/meta#7
annevk added a commit to whatwg/fetch that referenced this issue Feb 7, 2017
- Updates to the deduplicated deploy script; part of whatwg/meta#11
- As such, stops deploying commit snapshots for branches
- Fixes .travis.yml syntax; part of whatwg/meta#4
- Adds .gitattributes and .editorconfig; part of whatwg/meta#7
annevk added a commit to whatwg/infra that referenced this issue Feb 13, 2017
- Updates to the deduplicated deploy script; part of whatwg/meta#11
- As such, stops deploying commit snapshots for branches
- Fixes .travis.yml syntax; part of whatwg/meta#4
- Adds .gitattributes and .editorconfig; part of whatwg/meta#7
domenic pushed a commit to whatwg/infra that referenced this issue Feb 13, 2017
- Updates to the deduplicated deploy script; part of whatwg/meta#11
- As such, stops deploying commit snapshots for branches
- Fixes .travis.yml syntax; part of whatwg/meta#4
- Adds .gitattributes and .editorconfig; part of whatwg/meta#7
annevk added a commit to whatwg/storage that referenced this issue Feb 15, 2017
- Updates to the deduplicated deploy script; part of whatwg/meta#11
- As such, stops deploying commit snapshots for branches
- Fixes .travis.yml syntax; part of whatwg/meta#4
- Adds .gitattributes and .editorconfig; part of whatwg/meta#7
annevk added a commit to whatwg/notifications that referenced this issue Feb 15, 2017
- Updates to the deduplicated deploy script; part of whatwg/meta#11
- As such, stops deploying commit snapshots for branches
- Fixes .travis.yml syntax; part of whatwg/meta#4
- Adds .gitattributes and .editorconfig; part of whatwg/meta#7
annevk added a commit to whatwg/fullscreen that referenced this issue Feb 15, 2017
- Updates to the deduplicated deploy script; part of whatwg/meta#11
- As such, stops deploying commit snapshots for branches
- Fixes .travis.yml syntax; part of whatwg/meta#4
- Adds .gitattributes and .editorconfig; part of whatwg/meta#7
annevk added a commit to whatwg/mimesniff that referenced this issue Feb 15, 2017
- Updates to the deduplicated deploy script; part of whatwg/meta#11
- As such, stops deploying commit snapshots for branches
- Fixes .travis.yml syntax; part of whatwg/meta#4
- Adds .gitattributes and .editorconfig; part of whatwg/meta#7
annevk added a commit to whatwg/quirks that referenced this issue Feb 15, 2017
- Updates to the deduplicated deploy script; part of whatwg/meta#11
- As such, stops deploying commit snapshots for branches
- Fixes .travis.yml syntax; part of whatwg/meta#4
- Adds .gitattributes and .editorconfig; part of whatwg/meta#7
@annevk
Copy link
Member Author

annevk commented Mar 20, 2017

Closing this in favor of #21.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

3 participants