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

WIP initial reshuffling of opts parsing API for #23009 #23061

Closed
wants to merge 12 commits into from

Conversation

@ejmg
Copy link
Contributor

ejmg commented Mar 19, 2019

Note: This PR is not close to finished, I am pushing my code here for review and assistance.

This PR addresses the generation and parsing of command line options that are global versus specific, e.g. Gluten Embedded, by changing the from_cmdline_args() API by adding an opts argument that can be passed in by a specific embedder, separating the concerns and configurations from the global set of engine options.


  • ./mach build -d does not report any errors
  • ./mach test-tidy does not report any errors
  • These changes fix #23009(GitHub issue number if applicable)
  • There are tests for these changes OR
  • These changes do not require tests because ___

This change is Reviewable

@highfive
Copy link

highfive commented Mar 19, 2019

Heads up! This PR modifies the following files:

  • @paulrouget: ports/servo/non_android_main.rs, ports/servo/Cargo.toml
@jdm jdm changed the title initial reshuffling of opts parsing API for #23009 WIP initial reshuffling of opts parsing API for #23009 Mar 20, 2019
ejmg added 2 commits Mar 21, 2019
…nged return type of from_cmdline_args to getopts::Result
@jdm
Copy link
Member

jdm commented Mar 21, 2019

This is basically what I expected!

@bors-servo
Copy link
Contributor

bors-servo commented Mar 22, 2019

The latest upstream changes (presumably #22973) made this pull request unmergeable. Please resolve the merge conflicts.

@servo-wpt-sync
Copy link
Collaborator

servo-wpt-sync commented Mar 22, 2019

Error syncing changes upstream. Logs saved in error-snapshot-1553276912875.

@jdm
Copy link
Member

jdm commented Apr 5, 2019

Looks like the rebase did not go as expected :(

@jdm jdm removed the S-awaiting-review label Apr 5, 2019
@ejmg
Copy link
Contributor Author

ejmg commented Apr 5, 2019

It really did not, and it's even more embarrassing that I thought I had fixed my rebase issues when I pushed this.

School things unexpectedly took priority this past week and a half, but I was planning on asking a friend on how to rebase and fix my branch's history this weekend. Am I roadblocking?

@jdm
Copy link
Member

jdm commented Apr 5, 2019

Not at all! I was just going through the queue of PRs that I'm supposed to review and making sure that I wasn't roadblocking.

@jdm
Copy link
Member

jdm commented Jun 30, 2019

For the record, the --angle flag is another one that only matters in ports/glutin, and was added in #22856.

@ejmg
Copy link
Contributor Author

ejmg commented Jun 30, 2019

Alright, my friend @nucle0tides helped me undo the mess that was this branch. I'll be working and hopefully getting this done in the coming week or so!.

ejmg added 2 commits Jun 30, 2019
…cess. servo/lib.rs now uses opt::multiprocess() in Servo::new()
…atic value and gluten/main2.rs
components/config/opts.rs Outdated Show resolved Hide resolved
@jdm jdm removed the S-awaiting-review label Jul 3, 2019
@ejmg
Copy link
Contributor Author

ejmg commented Jul 6, 2019

@jdm I'm using the same pattern for all of these flags so far by making a global atomic bool to check/set from the embedder, including the new --angle. That work?

@jdm
Copy link
Member

jdm commented Jul 6, 2019

That still leaves the flag in components/config. For any flags that are only used in ports/ code, that's unnecessary.

@jdm
Copy link
Member

jdm commented Jul 6, 2019

To be more explicit - I would expect the angle opt parsing to cause a local use_angle variable to contain the result, and that variable can be passed to any code that previously used opts::angle(). That keeps embedding-specific details out of the rest of the engine.

@bors-servo
Copy link
Contributor

bors-servo commented Jul 8, 2019

The latest upstream changes (presumably #23712) made this pull request unmergeable. Please resolve the merge conflicts.

@jdm
Copy link
Member

jdm commented Oct 10, 2019

Closing due to inactivity.

@jdm jdm closed this Oct 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

5 participants
You can’t perform that action at this time.