Remove an useless reference to the deprecated qt toolkit

RESOLVED FIXED in Firefox 61

Status

enhancement
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: sylvestre, Assigned: dev.mbar, Mentored)

Tracking

Trunk
mozilla61

Firefox Tracking Flags

(firefox61 fixed)

Details

Attachments

(2 attachments, 4 obsolete attachments)

(Reporter)

Description

a year ago
We just to have a QT support in the past but it has been removed a while ago.
However, there is still a trace which should be removed:
https://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/installer/upload-files.mk?q=upload-files.mk&redirect_type=direct#15

I assigned this bug to a friend so that he can understand our processes.
(Assignee)

Updated

a year ago
Status: NEW → ASSIGNED
Comment hidden (mozreview-request)
(Reporter)

Comment 2

a year ago
mozreview-review
Comment on attachment 8944705 [details]
Bug 1429019 - Remove QT support code's reference.

https://reviewboard.mozilla.org/r/214860/#review220520

::: toolkit/mozapps/installer/upload-files.mk
(Diff revision 1)
>  MOZ_PKG_FORMAT  = ZIP
>  else
>  ifeq (,$(filter-out SunOS, $(OS_ARCH)))
>     MOZ_PKG_FORMAT  = BZ2
>  else
> -   ifeq (,$(filter-out gtk3 qt, $(MOZ_WIDGET_TOOLKIT)))

I think we have to keep the line
 ifeq (,$(filter-out gtk3 qt, $(MOZ_WIDGET_TOOLKIT)))
 
and simplify it:
ifeq (gtk3,$(MOZ_WIDGET_TOOLKIT))

::: toolkit/mozapps/installer/upload-files.mk
(Diff revision 1)
>  else
>  ifeq (,$(filter-out SunOS, $(OS_ARCH)))
>     MOZ_PKG_FORMAT  = BZ2
>  else
> -   ifeq (,$(filter-out gtk3 qt, $(MOZ_WIDGET_TOOLKIT)))
> -      MOZ_PKG_FORMAT  = BZ2

I think you changed the behavior here.
We still need
 MOZ_PKG_FORMAT  = BZ2
Attachment #8944705 - Flags: review?(sledru)
(Assignee)

Comment 3

a year ago
After reading you're comment, I just realized I've been a little bit too fast about the code (i was so happy to have compile everything) and I agree with you about what we have to keep and remove.
But, just for my understanding (and due to my lake of knownledege about this kind of code), are we sure qt case couldn't fall into the last "ifeq (android,$(MOZ_WIDGET_TOOLKIT))" which should still mean qt is supported ?
(Reporter)

Comment 4

a year ago
Don't worry about that. The options to tell to the build system that you want to build with qt has been removed. So, it is just fail way earlier in the build.
Comment hidden (mozreview-request)
(Reporter)

Comment 6

a year ago
mozreview-review
Comment on attachment 8946234 [details]
Bug 1429019 - Remove QT support code's reference.

https://reviewboard.mozilla.org/r/216200/#review222008

Please merge the commits into a single one:
https://reviewboard.mozilla.org/r/216200/diff/1
here, you have two commits. We should have only one with only one change.
feel free to ask me by email if you don't know how to do it.
Attachment #8946234 - Flags: review?(sledru) → review-
(Assignee)

Comment 7

a year ago
Yep, that's just what i saw. I will check on my side first.
Comment hidden (mozreview-request)
(Assignee)

Updated

a year ago
Attachment #8946234 - Attachment is obsolete: true
(Reporter)

Comment 9

a year ago
mozreview-review
Comment on attachment 8944705 [details]
Bug 1429019 - Remove QT support code's reference.

https://reviewboard.mozilla.org/r/214860/#review222014

::: commit-message-e4107:5
(Diff revision 2)
> +Bug 1429019 - Remove QT support code's reference. r?sylvestre
> +
> +MozReview-Commit-ID: LwTAoYmxkTP
> +***
> +Bug 1429019 - Remove QT support code's reference. r?sylvestre

Duplicate declaration :)

::: toolkit/mozapps/installer/upload-files.mk:15
(Diff revision 2)
>  MOZ_PKG_FORMAT  = ZIP
>  else
>  ifeq (,$(filter-out SunOS, $(OS_ARCH)))
>     MOZ_PKG_FORMAT  = BZ2
>  else
> -   ifeq (,$(filter-out gtk3 qt, $(MOZ_WIDGET_TOOLKIT)))
> +   ifeq (,$(filter-out gtk3, $(MOZ_WIDGET_TOOLKIT)))

As we have only one declaration, this could probably be simplified with
ifeq (gtk3,$(MOZ_WIDGET_TOOLKIT))
(like the cocoa declarations).

The rest should be also cleanup (but in a different bug)
Attachment #8944705 - Flags: review?(sledru)
(Assignee)

Comment 10

a year ago
For the use of filter-out, i kept it as it was already used in the other cases. But from my understanding, we could remove it to simplify the code as you pointed out.

About the double declaration, i'm not sure to know how to remove it as I guess it exist because of my last commit mistake.
Do I need to use a commit amend ?
(Reporter)

Comment 11

a year ago
(In reply to Matthieu Baratte from comment #10)
> For the use of filter-out, i kept it as it was already used in the other
> cases. But from my understanding, we could remove it to simplify the code as
> you pointed out.
A rule of thumb: question everything :)
It isn't because it is this way that it has to remain this was.

> About the double declaration, i'm not sure to know how to remove it as I
> guess it exist because of my last commit mistake.
> Do I need to use a commit amend ?
yes
hg commit --amend and remove duplicate content.
(after the first moz review id)
Comment hidden (mozreview-request)
(Assignee)

Comment 13

a year ago
Hi, working back on it. Don't check my last pull, i messed up and will do it back !
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 17

a year ago
I did something wrong with hg and don't succeed to correct it. I tried to correct on my own but probably mess even more ! 
I have 3 changeset (and it sounds like i even did a branch) and can't merge them thanks to histedit.

if you have any idea, it could be cool as I would like to do something clean.
(Reporter)

Comment 18

a year ago
Indeed :)

You should do the following:
* on your system, look at the "hg wip" tree and remove all your hcanges with "hg strip -r <revision>"
* In mozreview, abandon the changes https://reviewboard.mozilla.org/r/219596/diff/4#index_header
* Do the changes on your side but one commit = one change
  So, you have one commit to remove the qt string
  And another commit to remove the "ifeq (,$(filter-out"

is that clear?
(Reporter)

Updated

a year ago
Attachment #8950383 - Flags: review?(sledru)
(Assignee)

Comment 19

a year ago
Back from hollidays
I was thinking about the first part but I feared to mess things even more by doing that ;)

About the second step, i don't really understand how i do that: Do I have to do it the web interface or within my terminal ?

Step 3 sounds clear when step 2 is ok (hope so as hg give me some problem ;)
(Reporter)

Comment 20

a year ago
in the web interface, just start from scratch :)
(Assignee)

Comment 21

a year ago
mm still not clear ;) should i archive it ?
I don't remember to have done something with the web interface :p
(Reporter)

Comment 22

a year ago
try:
review summary => close => discarded
(Assignee)

Updated

a year ago
Attachment #8944705 - Attachment is obsolete: true
(Assignee)

Updated

a year ago
Attachment #8950383 - Attachment is obsolete: true
(Assignee)

Comment 23

a year ago
ok, i was too low in the tree ;)
i discarded everything and there I go back at the beginning ;)
(Assignee)

Comment 24

a year ago
ok, i was too low in the tree ;)
i discarded everything and there I go back at the beginning ;)
(Assignee)

Comment 25

a year ago
Hello, one last thing before to finsih and not to reiterate the same error:
1 commit for 1 change -> OK
But i will then have 2 commits again, so i will have to merge before to pull, right ?
(Reporter)

Comment 26

a year ago
Yes, 1 commit for a 1 change
one of the qt
the other for the comparisons

not sure what you mean by merge :)

Updated

a year ago
Product: Core → Firefox Build System
(Assignee)

Comment 27

a year ago
I think this is what i've done the first time and got 2 requests (so it would be the reason why i had to remove everything)
This is perhaps due to the fact i commit using vscode. I will try to do it with command line and we will see.
I guess i made most of the mistake about mercurial, so i should be able to correct myself now ;)
(Reporter)

Comment 28

a year ago
Otherwise, you can just attach the patch in bugzilla. ;)
(export with "hg export -r <revision> > foo1.diff")
(Assignee)

Comment 29

a year ago
yep but i have to do it good with commit.

My question is:
- do I do the first modif, commit then pull, then do the second one with the same process
- or do i do the first modif, commit, then do the second, commit and then merge / pull

And now i know what are the modificaitons, why couldn't I do them in only one commit ? is it to keep track of every modifications("correction" + "optimization")

Note: this is just to have the best process for the next time and I think it could be usefull in all my project :)
(Reporter)

Comment 30

a year ago
> - or do i do the first modif, commit, then do the second, commit and then merge / pull

I guess you meant "push" and not "pull" but yeah, do the all the local modifications and they push.

This is correct. We want "one commit = changing a one thing".
We could have even created a separate bug (but it would be a bit overkill)
Comment hidden (mozreview-request)
(Reporter)

Comment 32

a year ago
I still see only one commit?!
(Reporter)

Comment 33

a year ago
mozreview-review
Comment on attachment 8956963 [details]
Bug 1429019 - Remove QT support code's reference.

https://reviewboard.mozilla.org/r/225910/#review232010
Attachment #8956963 - Flags: review?(sledru)
(Assignee)

Comment 34

a year ago
I merged my 2 commits before to push :D
Guess i didn't understood what you told me.

So I guess you wanted the following sequence: "commit push commit push" ?

I did "commit commit merge push" as I feared to mess things as the first time.
So do I clean up again and do as needed ? :)

Btw I saw I still have one line when i'm doing an hg outgoing (after my last push). Is it normal ?
So if I'm doing "commit push commit push" sequence i guess it shoud be "commit push commit merge push" ? Is that right ?
(Assignee)

Comment 35

a year ago
As spoken by mail, i will reset everything and do it back as intended.
Will have to work on my side after all this to improve my knowledege ;)
(Assignee)

Updated

a year ago
Attachment #8956963 - Attachment is obsolete: true
Comment hidden (mozreview-request)
(Reporter)

Comment 38

a year ago
mozreview-review
Comment on attachment 8958638 [details]
Bug 1429019 - Simplify the comparaison by removing the use of filter-out.

https://reviewboard.mozilla.org/r/227548/#review233398

::: commit-message-f4e7e:1
(Diff revision 1)
> +Bug 1429019 - Code optimization. r?sylvestre

Please update the commit message for something more specific.
Probably something like:
Simplify the comparison by removing the use of filter-out (not used)
Attachment #8958638 - Flags: review?(sledru)
(Assignee)

Comment 40

a year ago
ok checking that :)
(Reporter)

Comment 41

a year ago
mozreview-review
Comment on attachment 8958637 [details]
Bug 1429019 - Remove QT support code's reference.

https://reviewboard.mozilla.org/r/227546/#review233436
Attachment #8958637 - Flags: review?(sledru)
(Reporter)

Comment 42

a year ago
ping?
Flags: needinfo?(dev.mbar)
(Assignee)

Comment 43

a year ago
hello! Have to work on it

Guess I have to do an histedit (to modify the last comment) and then push again ?
Flags: needinfo?(dev.mbar)
(Reporter)

Comment 44

a year ago
Correct
* histedit
* you replace "pick" by "mess"
and update the comment
then
hg push -r <revision_1>::<revision_2> review
et voila!
Comment hidden (mozreview-request)
(Assignee)

Updated

a year ago
Attachment #8958637 - Flags: review?(mh+mozilla)
Attachment #8958638 - Flags: review?(sledru) → review?(mh+mozilla)

Comment 46

a year ago
mozreview-review
Comment on attachment 8958638 [details]
Bug 1429019 - Simplify the comparaison by removing the use of filter-out.

https://reviewboard.mozilla.org/r/227548/#review238586
Attachment #8958638 - Flags: review?(mh+mozilla) → review+

Comment 47

a year ago
mozreview-review
Comment on attachment 8958637 [details]
Bug 1429019 - Remove QT support code's reference.

https://reviewboard.mozilla.org/r/227546/#review238588
Attachment #8958637 - Flags: review?(mh+mozilla) → review+

Comment 48

a year ago
Pushed by sledru@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/dbfef3131637
Remove QT support code's reference. r=glandium
https://hg.mozilla.org/integration/autoland/rev/a7c810ae6894
Simplify the comparaison by removing the use of filter-out. r=glandium

Comment 49

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/dbfef3131637
https://hg.mozilla.org/mozilla-central/rev/a7c810ae6894
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
You need to log in before you can comment on or make changes to this bug.