Closed Bug 1429019 Opened 8 years ago Closed 8 years ago

Remove an useless reference to the deprecated qt toolkit

Categories

(Firefox Build System :: General, enhancement)

Product:
Firefox Build System
Component:
General
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

(firefox61 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla61
Tracking Flags:
Tracking Status
firefox61 --- fixed

People

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

References

Details

Attachments

(2 files, 4 obsolete files)

Bug 1429019 - Remove QT support code's reference.
59 bytes, text/x-review-board-request
glandium
: review+
Details
Bug 1429019 - Simplify the comparaison by removing the use of filter-out.
59 bytes, text/x-review-board-request
glandium
: review+
Details
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.
Status: NEW → ASSIGNED
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)
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 ?
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 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-
Yep, that's just what i saw. I will check on my side first.
Attachment #8946234 - Attachment is obsolete: true
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)
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 ?
(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)
Hi, working back on it. Don't check my last pull, i messed up and will do it back !
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.
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?
Attachment #8950383 - Flags: review?(sledru)
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 ;)
in the web interface, just start from scratch :)
mm still not clear ;) should i archive it ? I don't remember to have done something with the web interface :p
try: review summary => close => discarded
Attachment #8944705 - Attachment is obsolete: true
Attachment #8950383 - Attachment is obsolete: true
ok, i was too low in the tree ;) i discarded everything and there I go back at the beginning ;)
ok, i was too low in the tree ;) i discarded everything and there I go back at the beginning ;)
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 ?
Yes, 1 commit for a 1 change one of the qt the other for the comparisons not sure what you mean by merge :)
Product: Core → Firefox Build System
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 ;)
Otherwise, you can just attach the patch in bugzilla. ;) (export with "hg export -r <revision> > foo1.diff")
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 :)
> - 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)
I still see only one commit?!
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)
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 ?
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 ;)
Attachment #8956963 - Attachment is obsolete: true
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)
ok checking that :)
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)
ping?
Flags: needinfo?(dev.mbar)
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)
Correct * histedit * you replace "pick" by "mess" and update the comment then hg push -r <revision_1>::<revision_2> review et voila!
Attachment #8958637 - Flags: review?(mh+mozilla)
Attachment #8958638 - Flags: review?(sledru) → review?(mh+mozilla)
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 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+
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
https://hg.mozilla.org/mozilla-central/rev/dbfef3131637 https://hg.mozilla.org/mozilla-central/rev/a7c810ae6894
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox61: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: