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

Chore/bump chromium webgl+kerberos #42751

Merged
merged 32 commits into from
Aug 27, 2019
Merged

Chore/bump chromium webgl+kerberos #42751

merged 32 commits into from
Aug 27, 2019

Conversation

joelgriffith
Copy link
Contributor

@joelgriffith joelgriffith commented Aug 6, 2019

Once chromium is built and SHA's generated I'll push them to our S3 bucket, but for now this includes updates to our typedefs (yaaa removing @ts-ignore), and uses a better @types package. I had to write a small wrapper around puppeteer to get the right module "binded" to the right types.

TODO:

  • Update puppeteer and fix puppeteer-core types.
  • Puppeteer@1.19.0.
  • Linux build.
  • Mac build.
  • Windows build.
  • Ensure maps are rendering
  • Upload binaries and SHA's to S3.
  • Add the above path's and SHA checksums into this PR.

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

Copy link
Member

@tsullivan tsullivan left a comment

Choose a reason for hiding this comment

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

Left a suggestion to do without the new puppeteer.ts file.

The maps app changes should be approved by someone in the Maps team.

import rimraf from 'rimraf';
import * as Rx from 'rxjs';
import { map, share, mergeMap, filter, partition, ignoreElements, tap } from 'rxjs/operators';
import { InnerSubscriber } from 'rxjs/internal/InnerSubscriber';

import { launch, Browser, Page } from '../puppeteer';
Copy link
Member

Choose a reason for hiding this comment

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

We can also do

import { Browser, Page, LaunchOptions } from 'puppeteer';

And when we call launch we can do:

        browser = await launch({
          pipe: !this.browserConfig.inspect,
          userDataDir,
          executablePath: this.binaryPath,
          ignoreHTTPSErrors: true,
          args: chromiumArgs,
          env: {
            TZ: browserTimezone,
          },
        } as LaunchOptions);

That will ensure that we are doing a good job at building the object literal that gets passed. We wouldn't need the puppeteer file, but on the other hand, the launch method would not be typed quite as well as you have it.

@elasticmachine
Copy link
Contributor

💔 Build Failed

@joelgriffith joelgriffith changed the title WIP: Chore/bump chromium webgl kerberos Chore/bump chromium webgl+kerberos Aug 26, 2019
@joelgriffith
Copy link
Contributor Author

Looks like the linux build didn't zip properly, going to fix that and re-upload the binary to S3

@elasticmachine
Copy link
Contributor

💔 Build Failed

@nreese
Copy link
Contributor

nreese commented Aug 26, 2019

Does GisMap component still need to set data-render-complete attribute now that CustomEvent(RENDER_COMPLETE_EVENT) is getting triggered?

@joelgriffith
Copy link
Contributor Author

joelgriffith commented Aug 26, 2019

No, I'll remove that attribute @nreese, good catch

EDIT: They are still required, sadly.

@tsullivan tsullivan self-requested a review August 26, 2019 22:29
archive_file('Helpers/chrome_crashpad_handler')
archive_file('libswiftshader_libEGL.dylib')
archive_file('libswiftshader_libGLESv2.dylib')
archive_file(path.join('Helpers', 'chrome_crashpad_handler'))
Copy link
Member

Choose a reason for hiding this comment

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

this is existing, but stands out a little, and I'm wondering why we have it in the Darwin build

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll have to look again. The binary fails to start wo it, however I'm not sure as to why

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@@ -3,7 +3,7 @@
* or more contributor license agreements. Licensed under the Elastic License;
* you may not use this file except in compliance with the Elastic License.
*/
import { Browser } from 'puppeteer';
Copy link
Member

Choose a reason for hiding this comment

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

this is essentially the same as the previous code, just an indirect version

Copy link
Member

@tsullivan tsullivan left a comment

Choose a reason for hiding this comment

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

I just have 1 picky point about how role of the new puppeteer file. I think it could be focused on exporting a typed launch function

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@tsullivan tsullivan self-requested a review August 27, 2019 16:30
Copy link
Member

@tsullivan tsullivan left a comment

Choose a reason for hiding this comment

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

LGTM! Reviewed the non-Maps part of the code changes.

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@joelgriffith joelgriffith merged commit 8b55ff6 into elastic:master Aug 27, 2019
joelgriffith pushed a commit that referenced this pull request Aug 28, 2019
* Chore/bump chromium webgl+kerberos (#42751)

* WIP: Adding libs for webgl

* WIP Adding swiftshader libs to chromium

* WIP: Adding missing binaries for webgl in chromium

* Use pipes for communication with chrome to avoid networking snafus

* Bumps puppeteer in prep for new chromium build + types and better @types package

* Remove ignore

* Removing of final @ts-ignore now that we have types

* README updates

* Fixing binding issues

* Fixing maps integration wrt reporting + conditional pipes for puppeteer

* Adding new deps to the windows build

* New s3 builds

* Checksums for updated linux build

* Moving types out of puppeteer file and into core puppeteer module

* launch => puppeteerLaunch

* Maps comment about render loading in reporting

* Clarify how reporting uses hooks and events for viz

* Type fix from merge conflict
@alexfrancoeur
Copy link

🍾 🍾 🍾 🍾

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
(Deprecated) Feature:Reporting Use Reporting:Screenshot, Reporting:CSV, or Reporting:Framework instead release_note:enhancement v7.4.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants