Closed Bug 1698595 Opened 3 years ago Closed 3 years ago

Remove Places-only sqrt SQL function and use the official one

Categories

(Toolkit :: Places, task, P3)

Product:
Toolkit
Component:
Places
Type:
task

Tracking

()

Status:
RESOLVED FIXED
Milestone:
89 Branch
Tracking Flags:
Tracking Status
firefox89 --- fixed

People

(Reporter: mak, Assigned: garima.mazumdar)

References

Details

(Keywords: good-first-bug, Whiteboard: [lang=cpp])

Attachments

(7 files, 1 obsolete file)

What I'm getting when I run ./mach build
280.47 KB, image/png
Details
Bug 1698595 - Remove Places-only sqrt SQL function and use the official SQLite one. r=mak
48 bytes, text/x-phabricator-request
Details | Review
hg log.png
142.31 KB, image/png
Details
2021-04-12.png
45.30 KB, image/png
Details
hg-wip2
287.71 KB, image/png
Details
hg-wip3
269.77 KB, image/png
Details
Bug 1698595 - Link SQLite and Math. r=asuth
48 bytes, text/x-phabricator-request
Details | Review

Sqlite.3.35.1 comes with math extension, we can compile with SQLITE_ENABLE_MATH_FUNCTIONS and then remove our sqrt() implementation https://searchfox.org/mozilla-central/rev/aa9a7136835deb0eeba00c62bb50a4a0e2cdea2d/toolkit/components/places/Database.cpp#1509-1510

This is a good first cpp bug, we must add -DSQLITE_ENABLE_MATH_FUNCTIONS to the sqlite compile options in third_party/sqlite3/src/moz.build, and remove the sqrt implementation in places/SQLFunctions.cpp/h
Running the tests in the sqlite and toolkit/components/places/ folders should be enough.

SQLite3.35.4 (dep) must have landed

Keywords: good-first-bug
Whiteboard: [lang=cpp]

Hi! I'm Garima, an outreachy participant, and I'd like to work on this bug :)

Feel free to work on it. I'll assign the bug once there's a first patch attached. If you have questions feel free to needinfo me.
I suggest to actually build Firefox (full build, no artifact, due to cpp dependencies) to verify the fix, and run the tests locally as suggested.

Can i work on this

I've done the changes but I'm not sure about how to run the tests...could you let me know what to do?

Flags: needinfo?(mak)

(In reply to Garima Mazumdar from comment #5)

I've done the changes but I'm not sure about how to run the tests...could you let me know what to do?

If you were able to build, you can run tests using the "mach" helper that is shipped with mozilla-central. From the src root you can issue "./mach test toolkit/components/places/" and "./mach test toolkit/storage/" the tests should all PASS.
There are other useful commands like ./mach run to execute the build. It also includes an help command.
There's additional automated tests documentation available at https://firefox-source-docs.mozilla.org/devtools/tests/README.html

Flags: needinfo?(mak)

I ran the tests in "./mach test toolkit/components/places/" and they all passed, but I don't have a storage folder in toolkit (I'm using windows if that matters) so what should I do now?

Flags: needinfo?(mak)

my memory failed, the folder is only named "storage/" (even if the Bugzilla component is Toolkit / Storage), sorry about that. You can run "./mach test storage/"

Last question, ensure your .mozconfig is not configured to do an artifact build, because those don't compile C++ code. If it contains --enable-artifact-builds it's not ok.

Flags: needinfo?(mak)

Once you have verified, you can ensure the commit message on the changeset you made is correct, it should read like "Bug 1698595 - Remove Places-only sqrt SQL function and use the official SQLite one. r=mak" and then you can push the changeset using moz-phab.
See: https://moz-conduit.readthedocs.io/en/latest/phabricator-user.html
Another example tutorial here: https://moz-conduit.readthedocs.io/en/latest/walkthrough.html

So after disabling the artifact build and running mach build and the tests, I keep getting errors and it runs 0 tests and says it looks like my program hasn't been built even though I've run mach build multiple times now, and tried the tests multiple times as well

Flags: needinfo?(mak)

Did you run ./mach bootstrap to install any necessary dependencies?
pre-requisites and dependencies are explained for each OS in https://firefox-source-docs.mozilla.org/contributing/contribution_quickref.html

./mach build must terminate with a success before you can run tests, if it fails it may either be because some prerequisite is missing, or the code changes are wrong, usually the log it prints should help to understand where it stopped.

Flags: needinfo?(mak)

I ran ./mach bootstrap for Firefox for Desktop, then ran ./mach build

Flags: needinfo?(mak)
Assignee: nobody → garima.mazumdar
Status: NEW → ASSIGNED

Hi, just wanted to let you know that I have submitted the first patch for this bug. Could you let me know what I should do after this?

Just wait, I'm sorry I was a bit overloaded, I'll handle the review in the next days.

The error in comment 12 was a code mistake, indeed the log pointed at issues in places/Database.cpp

Flags: needinfo?(mak)

Sure no issues, and I've already handled the errors I'd gotten and committed the code :)

Flags: needinfo?(mak)

I left review comments in Phabricator, feel free to update the changeset and submit again, that will automatically update the attached phabricator review (automatically asking me again for review). I left some instructions in Phabricator to help following the correct procedure, the tutorials I linked above may also help. in case of issues just ping me with questions.

Flags: needinfo?(mak)

Hi, just wanted to know what comment to add while committing the changes

Flags: needinfo?(mak)

Hi, by mistake I made a completely new commit instead of adding on to this bug, could you tell me how I could fix that?

(In reply to Garima Mazumdar from comment #20)

Hi, just wanted to know what comment to add while committing the changes

the comment on updating a changeset is not mandatory, unless you want the reviewer to know why you are making certain changes, you can also use a generic "addressing review comments" comment.

(In reply to Garima Mazumdar from comment #21)

Hi, by mistake I made a completely new commit instead of adding on to this bug, could you tell me how I could fix that?

you can merge changesets using hg rebase -s source -d destination --collapse, for example if you have cset2 and cset 3 on top of cset1, you can hg rebase -s cset2 -d cset1 --collapse, that will collapse cset2 and cset3 into a new single changeset, and open an editor containing all the commit titles, you can just edit that and keep the right commit title.

Flags: needinfo?(mak)

The issue is that I'd been working on another bug in between, so I need to merge changeset1 and changset3, so how do I do that? Someone had told me to try hg histedit, but I'm not sure how to go about it exactly

Flags: needinfo?(mak)

There are many different ways to handle that, I can tell you what I usually do.
Suppose you have

| bug1-cset3
| bug2-cset4
| bug1-cset2
cset1

you can rebase bug1-cset3 on top of bug1-cset2 (hg rebase -s bug1-cset3 -d bug1-cset2) , and you can then rebase bug1-cset4 on top of cset1. this way you end up with 2 heads

|bug1-cset3  | bug2-cset4
|bug1-cset2  |
|         /
...
|/
cset 1

Ad that point you can rebase--collapse bug1 csets, you'll still have 2 heads it's not a problem, it's a feature! You can hg update to a specific head to continue working on that bug. You use hg wip to visualize your heads, and can rebase things at any time. I use one head per bug, even if each head can have multiple changesets or bugs of course.

Flags: needinfo?(mak)

Hi, I'm so sorry but I made a mistake again. So basically this is what my hg commit looks like:
bug1-c2
bug2-c2
bug2->wrong commit, so this is not connected to any bug
bug1-c1

So now, I wanted to combine bug1-c1 and bug2-c2 but got confused and rebased bug1-c2 on top of bug2->wrong commit, please tell me how I can fix this? :(

Flags: needinfo?(mak)

The idea is that you must separate each bug into different heads, before being able to do any kind of collapsing. Thus you must have bug1 in one head, bug 2 in another head, and hg wip should show those 2 heads clearly.
Let's suppose the parent of bug1_c1 is mozilla-central, in your case you should

hg rebase -s bug1-c2 -d bug1-c1 (move bug1-c2 on top of bug1-c1)
hg rebase -s bug2_wrong -d central (move bug2_wrong and bug2-c2 on top of central)

I don't know what "wrong commit" is, is it code to throw away or collapse into bug2-c2? Anyway at that point you can use --collapse on each head

Flags: needinfo?(mak)
Attached image hg log.png —

So I rebased the latest commit (addressed review comments) on top of a wrong patch I had made (which was later marked obsolete)

Flags: needinfo?(mak)
Attached image 2021-04-12.png —

and this is how my hg wip looks

Now I'm confused about how to separate the two heads I rebased since I can't see the Latest Commit head in hg wip

based on what I see in hg wip:

hg rebase -s 642130 -d 642120 // (move 642130 on top of 642120)
hg rebase -s 642128 -d 642126 // (move 642128 on top of 642126)

Now check hg wip, if it looks good

hg rebase -s 642120 -d parent_of_642120 --collapse

replace parent_of_642120 with the actual cset number

Flags: needinfo?(mak)

For the next time I suggest to hg update to the patch you want to modify, do the changes and then use hg amend to update the changeset in place, rather than create a new commit. All the problems can be fixed with rebase and collapse anyway.

Attached image hg-wip2 —

want to get rid of the [devtools] commit

Flags: needinfo?(mak)

Hi, just one last doubt, as you can see from the above attachment, I don't really need 642126 so I wanted to know if there was any way to get rid of it or ensure that I do not commit anything to it again? And also I was doing hg rebase -s 642128 -d 642126 but since I needed to merge some conflicts to it which were not necessary to it, I didn't go through with that rebase, so what should I do now?

you can always hg prune draft changesets that you don't want anymore, but they should be at a head, so you should first rebase its children, or prune the children too.
In this case 642126 has no children, so you can just hg prune it. The changes into it will be lost.
here you should also still rebase 642128 on top of 642100, so you have 2 clean heads, one per bug

Flags: needinfo?(mak)
Attached image hg-wip3 —
Flags: needinfo?(mak)

just wanted to cross-check once, I should do hg rebase -s 642131 -d 642132 --collapse right?

(In reply to Garima Mazumdar from comment #36)

just wanted to cross-check once, I should do hg rebase -s 642131 -d 642132 --collapse right?

No, -s should be the "oldest" changeset and it will bring all its children with it. -d is the destination head, so the parent changeset.
You want to rebase --collapse 642120 and its child 642131 into a single changeset, the resulting single changeset should still be on top of 642100.
So the right command is hg rebase -s 642120 -d 642100 --collapse (collapse the whole head that starts with 642120 on top of 642100)

Flags: needinfo?(mak)

I've done the above step, so now should I do moz-submit cset cset -m "Addressed review comments" ?

Flags: needinfo?(mak)

Check that the commit message is still correct "Bug 1698595 - Remove Places-only sqrt SQL function and use the official SQLite one. r=mak" and that the mozreview ID remained in the commit message (it is usually added automatically by moz-phab a few lines below the commit message). You can modify it with hg update to that changeset and then issuing "hg commit --amend". You can also make no changes if it's correct.
Then you moz-phab submit, as you said.

Flags: needinfo?(mak)

Hi! Thanks for all the help and patience. I've added all the changes, please let me know how I should proceed :)

Flags: needinfo?(mak)

just follow the review comments in Phabricator, you don't need to needinfo me for that because phabricator already sends me a ping when you update the changeset. In general you should use needinfo when you have questions, changes in Phabricator already notify the interested people.

Flags: needinfo?(mak)

Linux Gcc requires -lm to properly link the math library.

I'm adding a small change on top for Linux, because it doesn't compile without passing -lm to the linker

Pushed by mak77@bonardo.net:
https://hg.mozilla.org/integration/autoland/rev/063efdbdbb14
Remove Places-only sqrt SQL function and use the official SQLite one. r=mak
https://hg.mozilla.org/integration/autoland/rev/7499c42b9196
Link SQLite and Math. r=asuth

Backed out 2 changesets (Bug 1698595) for causing android bustages.
Backout link: https://hg.mozilla.org/integration/autoland/rev/bbf5d711bbc8534476c1a49f82b9e0706db07c01
Push with failures, failure log.

Flags: needinfo?(mak)

It looks like also Android requires linking math. I actually thought Android was using Rust Sqlite, my fault.

Flags: needinfo?(mak)
Pushed by mak77@bonardo.net:
https://hg.mozilla.org/integration/autoland/rev/5f59ab7cc59c
Remove Places-only sqrt SQL function and use the official SQLite one. r=mak
https://hg.mozilla.org/integration/autoland/rev/2e5ef19cd699
Link SQLite and Math. r=asuth

https://hg.mozilla.org/mozilla-central/rev/5f59ab7cc59c
https://hg.mozilla.org/mozilla-central/rev/2e5ef19cd699

Status: ASSIGNED → RESOLVED
Closed: 3 years ago
status-firefox89: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → 89 Branch
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: