Skip to content

/ servo

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

Adding default config directories. #10498

Merged
merged 1 commit into from May 25, 2016
Merged

Adding default config directories. #10498

merged 1 commit into from May 25, 2016
+170 −44

Conversation

@creativcoder
Copy link
Contributor

creativcoder commented Apr 9, 2016

Fixes #10338

This change is Reviewable
components/servo/Cargo.lock Outdated
@@ -2214,6 +2216,15 @@ dependencies = [

[[package]]
name = "uuid"
version = "0.1.18"

This comment has been minimized.

Sign in to view
Copy link
@Manishearth

Manishearth Apr 9, 2016

Member

We shouldn't depend on two versions of the same crate if we can help it.

This comment has been minimized.

Sign in to view
Copy link
@creativcoder

creativcoder Apr 9, 2016

Author Contributor

Ok. So 0.1.18, needs to be removed and uuid 0.2.0 needs to stay; right ?
@creativcoder creativcoder changed the title For #10338 Adding default config directories. Adding default config directories. Apr 9, 2016
@creativcoder creativcoder force-pushed the creativcoder:default-profile-dir branch from 71ccfe9 to a2add1f Apr 9, 2016
@KiChjang
Copy link
Member

KiChjang commented Apr 9, 2016

@creativcoder Also, just a small nit, in your PRs, please say something like "fixes #10338" or "addresses #10338" or "closes #10338" instead of "for #10338". This will cause GitHub to automatically close the issue number you stated once the PR lands.
components/util/Cargo.toml Outdated
@@ -45,7 +45,9 @@ serde = "0.7"
serde_macros = "0.7"
smallvec = "0.1"
string_cache = {version = "0.2.11", features = ["heap_size"]}
url = {version = "0.5.7", features = ["heap_size", "serde_serialization"]}
url = {version = "0.5.8", features = ["heap_size", "serde_serialization"]}
uuid = "0.1.17"

This comment has been minimized.

Sign in to view
Copy link
@Ms2ger

Ms2ger Apr 9, 2016

Contributor

I removed the dependency on uuid here, please don't add it back.
@creativcoder creativcoder force-pushed the creativcoder:default-profile-dir branch 2 times, most recently from f0f1cc9 to fe70b43 Apr 10, 2016
@aneeshusa
Copy link
Member

aneeshusa commented Apr 11, 2016

This should also remove the --profile-dir option, since it's being replaced by --config-dir.
@creativcoder
Copy link
Contributor Author

creativcoder commented Apr 11, 2016

@aneeshusa Ok. I assumed that --config-dir was an additive option. I'll make the necessary changes then.
@creativcoder creativcoder force-pushed the creativcoder:default-profile-dir branch from fe70b43 to 99107f3 Apr 11, 2016
@SimonSapin
Copy link
Member

SimonSapin commented Apr 11, 2016

Reviewed 4 of 4 files at r3, 2 of 3 files at r4.
Review status: 6 of 7 files reviewed at latest revision, 8 unresolved discussions, some commit checks failed.

components/util/basedir.rs, line 18 [r4] (raw file):
Shouldn’t this and other functions in this file be based on the --config-dir command-line option? We don’t want to touch the default location if another one is specified.

components/util/basedir.rs, line 21 [r4] (raw file):
Shouldn’t this use with_profile?

components/util/basedir.rs, line 53 [r4] (raw file):
Libraries in general should not swallow errors or print to stderr. Instead return a Result and let the caller decide how to deal with errors.

components/util/basedir.rs, line 59 [r4] (raw file):
Is this line unused?

tests/unit/util/prefs.rs, line 50 [r4] (raw file):
Use let _ = something_that_returns_result; instead of this to ignore errors. But tests should generally not ignore errors, so use something_that_returns_result.unwrap(); instead.

tests/unit/util/prefs.rs, line 63 [r4] (raw file):
Nit: why the leading _? That’s usually used to suppress "unused variable" warnings, but this is used here.

Comments from Reviewable
@bors-servo
Copy link
Contributor

bors-servo commented Apr 12, 2016

☔ The latest upstream changes (presumably #10527) made this pull request unmergeable. Please resolve the merge conflicts.
@creativcoder creativcoder force-pushed the creativcoder:default-profile-dir branch 2 times, most recently from 37292b6 to 458d487 Apr 12, 2016
@creativcoder
Copy link
Contributor Author

creativcoder commented Apr 12, 2016

@SimonSapin I made changes.

components/util/basedir.rs, line 18 [r4] (raw file):
Can you elaborate on this one ? I am not clear on the intention here. As for using these functions; only the bootstrap_default_dirs is being called, when setting the initial environment options ( method from_cmdline_args in opts.rs) for servo, the other functions are meant to be called only ( in prefs.rs ) when checking if this flag is present there. Correct me if that is not the right place to call, bootstrap_default_dirs.

Comments from Reviewable
@creativcoder creativcoder force-pushed the creativcoder:default-profile-dir branch 2 times, most recently from 0ae94d4 to 98bf769 Apr 12, 2016
@bors-servo
Copy link
Contributor

bors-servo commented Apr 18, 2016

☔ The latest upstream changes (presumably #10682) made this pull request unmergeable. Please resolve the merge conflicts.
@creativcoder creativcoder force-pushed the creativcoder:default-profile-dir branch from 98bf769 to 27f8ce0 Apr 19, 2016
@bors-servo
Copy link
Contributor

bors-servo commented Apr 24, 2016

☔ The latest upstream changes (presumably #10670) made this pull request unmergeable. Please resolve the merge conflicts.
@creativcoder creativcoder force-pushed the creativcoder:default-profile-dir branch from 27f8ce0 to dea8036 Apr 25, 2016
@bors-servo
Copy link
Contributor

bors-servo commented Apr 26, 2016

☔ The latest upstream changes (presumably #10842) made this pull request unmergeable. Please resolve the merge conflicts.
@SimonSapin
Copy link
Member

SimonSapin commented Apr 26, 2016

Review status: 0 of 10 files reviewed at latest revision, 8 unresolved discussions, some commit checks failed.

components/util/basedir.rs, line 18 [r4] (raw file):
My point is: why does bootstrap_default_dirs exist at all? If I’m running servo --config-dir ~/path/to/config/servo then ~/.config/servo should not be created at all.

Comments from Reviewable
@paulrouget
Copy link
Contributor

paulrouget commented May 15, 2016

@creativcoder any update on this?
@creativcoder
Copy link
Contributor Author

creativcoder commented May 15, 2016 •
edited

Hi @paulrouget @SimonSapin ! I made changes. I've removed the usage of bootstrap_default_dirs, from the relevant places, and removed the redundant code from add_user_prefs. Let me know if these changes need more corrections.
@creativcoder creativcoder force-pushed the creativcoder:default-profile-dir branch 2 times, most recently from ce0a9d9 to 9ff612a May 15, 2016
@jdm jdm removed the S-needs-rebase label May 15, 2016
@creativcoder creativcoder force-pushed the creativcoder:default-profile-dir branch from 9ff612a to 3bc514b May 16, 2016
@highfive
Copy link

highfive commented May 16, 2016

New code was committed to pull request.
@creativcoder creativcoder force-pushed the creativcoder:default-profile-dir branch from 3bc514b to 2bb9f27 May 16, 2016
@highfive
Copy link

highfive commented May 16, 2016

New code was committed to pull request.
@bors-servo
Copy link
Contributor

bors-servo commented May 20, 2016

☔ The latest upstream changes (presumably #11189) made this pull request unmergeable. Please resolve the merge conflicts.
@creativcoder creativcoder force-pushed the creativcoder:default-profile-dir branch from 2bb9f27 to 6ee5638 May 23, 2016
@SimonSapin
Copy link
Member

SimonSapin commented May 24, 2016

Looks good, thanks!

@bors-servo r+
@bors-servo
Copy link
Contributor

bors-servo commented May 24, 2016

📌 Commit 6ee5638 has been approved by SimonSapin
@bors-servo
Copy link
Contributor

bors-servo commented May 24, 2016

⌛ Testing commit 6ee5638 with merge 57872a4...
bors-servo added a commit that referenced this pull request May 24, 2016
@bors-servo
Auto merge of #10498 - creativcoder:default-profile-dir, r=SimonSapin
57872a4 
Adding default config directories.

Fixes #10338
<!-- Reviewable:start -->
This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/10498)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented May 24, 2016

💔 Test failed - linux-dev
@SimonSapin
Copy link
Member

SimonSapin commented May 24, 2016

@creativcoder This needs you to run ./mach cargo-update -p util and commit the changes.
@creativcoder
adding default config dirs
Loading status checks…
b4885fe
@creativcoder creativcoder force-pushed the creativcoder:default-profile-dir branch from 6ee5638 to b4885fe May 25, 2016
@highfive
Copy link

highfive commented May 25, 2016

New code was committed to pull request.
@SimonSapin
Copy link
Member

SimonSapin commented May 25, 2016

@bors-servo r+
@bors-servo
Copy link
Contributor

bors-servo commented May 25, 2016

📌 Commit b4885fe has been approved by SimonSapin
@bors-servo
Copy link
Contributor

bors-servo commented May 25, 2016

⌛ Testing commit b4885fe with merge affc110...
bors-servo added a commit that referenced this pull request May 25, 2016
@bors-servo
Auto merge of #10498 - creativcoder:default-profile-dir, r=SimonSapin
Loading status checks…
affc110 
Adding default config directories.

Fixes #10338
<!-- Reviewable:start -->
This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/10498)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented May 25, 2016

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel, mac-dev-unit, mac-rel-css, mac-rel-wpt, windows
@bors-servo bors-servo merged commit b4885fe into servo:master May 25, 2016
3 checks passed
3 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
@bors-servo bors-servo mentioned this pull request May 25, 2016
Merged
3 of 5 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@SimonSapin SimonSapin

Labels
None yet
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

None yet

You can’t perform that action at this time.