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

Update chromium source #34

Merged
merged 52 commits into from
Jul 23, 2018
Merged

Update chromium source #34

merged 52 commits into from
Jul 23, 2018

Conversation

malloxpb
Copy link
Member

@malloxpb malloxpb commented Jul 18, 2018

This PR aims to update the chromium source that SCURL is using

@lopuhin
Copy link
Member

lopuhin commented Jul 19, 2018

@nctl144 I'm getting the same error locally as here on travis: https://travis-ci.org/nctl144/scurl/jobs/405492728 - probably you didn't push some commits to https://github.com/nctl144/url-chromium

@malloxpb malloxpb changed the title Update source new Update chromium source Jul 20, 2018
@malloxpb
Copy link
Member Author

The build is green now @lopuhin!

@codecov
Copy link

codecov bot commented Jul 20, 2018

Codecov Report

Merging #34 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master      #34   +/-   ##
=======================================
  Coverage   88.46%   88.46%           
=======================================
  Files           2        2           
  Lines         312      312           
=======================================
  Hits          276      276           
  Misses         36       36

@lopuhin
Copy link
Member

lopuhin commented Jul 23, 2018

Hey @nctl144 nice job getting it to work! Now that you know about all issues and which bits are required, will it be possible to create another branch in the subrepos that avoids commits like malloxpb/url-chromium@df10da1 and scrapy/base-chromium@5c86b03, and keeps changes to the minimum? The reason is that if upstream changes files that you removed, then you'll get a conflict (it not too hard to resolve, but it's better if there are no such conflicts).

Also I wonder where did malloxpb/url-chromium@e07c4bc come from, is it copied from somewhere? If yes, would be nice to put the source in the commit message and/or the file itself.

Also I'm curious why malloxpb/url-chromium@b6abe2b was needed? Also I'm a bit worried about scrapy/base-chromium@a61a1f5

If you prefer, it's possible to defer solving this issues, although if they don't take too much time to solve, it would be better to solve them now IMO :)

I'll review the rest of the PR in a minute.

Copy link
Member

@lopuhin lopuhin left a comment

Choose a reason for hiding this comment

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

The changes in this PR itself look good, and I'm fine with merging it now, but please see comments above regarding changes in the submodules - it could be better to fix them while your memory about this issues is fresh.

@malloxpb
Copy link
Member Author

malloxpb commented Jul 23, 2018

Also I'm a bit worried about scrapy/base-chromium@a61a1f5

Hey @lopuhin , I believe lock functionality is different depends on the OS platform that the users have, and it is not used in any of the functions that GURL components have..

Also I'm curious why malloxpb/url-chromium@b6abe2b was needed?

This is not needed but I commented it out to make it easier to get which source .cc files I needed to include (it help narrow down the scope a little bit), but I will put it back now!

I think I'm going to make a new branch which has all the original files and only make changes to the files that we need, if that's okay :) The submodules are kinda messy right now indeed...

@lopuhin
Copy link
Member

lopuhin commented Jul 23, 2018

I think I'm going to make a new branch which has all the original files and only make changes to the files that we need, if that's okay :)

Yeah, that would be perfect! 👍

@malloxpb
Copy link
Member Author

malloxpb commented Jul 23, 2018

Hey @lopuhin , I just updated the submodules so they have the same structure with the upstream. I just modified the files that we need :) I will work on updating the doc now.

In addition, the scurl branch of those 2 submodules are the old branches, while the new branches that we officially use are named scurl_clean...

@lopuhin
Copy link
Member

lopuhin commented Jul 23, 2018

Thanks @nctl144 , that's massively cleaner and better now! Here are the links I used to check the changes: malloxpb/url-chromium@upstream...scurl_clean and scrapy/base-chromium@master...scurl_clean

@malloxpb
Copy link
Member Author

malloxpb commented Jul 23, 2018

I just changed the branch names to make it less confusing... I changed the offical branches that we are using to 'master' and the upstream branch to 'upstream'. So the links are going to be:
nctl144/[email protected] and nctl144/[email protected]

@malloxpb
Copy link
Member Author

Hey @lopuhin , I will merge this PR now and I will create an issue for the tox problem on Mac OS that I found :)

@malloxpb malloxpb merged commit fef1b82 into master Jul 23, 2018
@malloxpb malloxpb deleted the update_source_new branch July 23, 2018 19:21
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