Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1158093002:
Add mips32 to LLVM configure's list of allowed Subzero targets (Closed)
|
Created:
5 years, 7 months ago by reedkotler Modified:
5 years, 6 months ago CC:
native-client-reviews_googlegroups.com, rich.fuhler_imgtec.com Base URL:
https://chromium.googlesource.com/native_client/pnacl-llvm.git@master Target Ref:
refs/heads/master Visibility:
Public. |
DescriptionAdd mips32 to LLVM configure's list of allowed Subzero targets.
BUG= https://code.google.com/p/nativeclient/issues/detail?id=4167
Patch Set 1 #Messages
Total messages: 14 (1 generated)
reedkotler@gmail.com changed reviewers: + jvoung@chromium.org, stichnot@chromium.org
Could you update the change list description on this codereview.chromium.org page? Git will take that description for its log entry, so it's good to get it right before landing =) The usual style is: ======= BEGIN template ==== one-liner description -- same as the "subject" in codereview more lines of description as needed. another line of description. more stuff. BUG= https://code.google.com/p/nativeclient/issues/detail?id=4167 ======= END template ==== The main thing is update the "BUG= ..." to the link. Otherwise, for this patch, I think a more specific one-liner could be more useful. E.g., the one-liner could be something like "Add mips32 to LLVM configure's list of allowed Subzero targets"? For ordering the patches, I'm not sure if you thought about it much, but I think this LLVM repo patch could probably go first. It won't take effect until the NaCl repo patch lands so it's pretty safe. The NaCl repo change needs to land *after* the Subzero repo patch (otherwise Subzero won't know how to build the new target that you enabled via configure / CMake). Also, we are in the middle of debugging some LLVM 3.7 merge bugs, so the LLVM tree is a bit "red" right now (see the red bots here: https://codereview.chromium.org/1160803002/). Other info: we also have "trybots" which, if you have trybot access, can help test LLVM changes before they land (e.g., on Linux, Mac and Windows to help cover machine types that you might not have access to). See native_client/pnacl/try_git_change.py "Example usage" section. This patch looks pretty safe, but for bigger patches that trybot script could be useful. Otherwise, this patch LGTM.
On 2015/05/28 00:10:59, jvoung wrote: > Could you update the change list description on this http://codereview.chromium.org > page? Git will take that description for its log entry, so it's good to get it > right before landing =) > > > The usual style is: > > ======= BEGIN template ==== > one-liner description -- same as the "subject" in codereview > > more lines of description as needed. > another line of description. > more stuff. > > BUG= https://code.google.com/p/nativeclient/issues/detail?id=4167 > ======= END template ==== > > The main thing is update the "BUG= ..." to the link. > > Otherwise, for this patch, I think a more specific one-liner could be more > useful. E.g., the one-liner could be something like "Add mips32 to LLVM > configure's list of allowed Subzero targets"? > > For ordering the patches, I'm not sure if you thought about it much, but I think > this LLVM repo patch could probably go first. It won't take effect until the > NaCl repo patch lands so it's pretty safe. The NaCl repo change needs to land > *after* the Subzero repo patch (otherwise Subzero won't know how to build the > new target that you enabled via configure / CMake). > > Also, we are in the middle of debugging some LLVM 3.7 merge bugs, so the LLVM > tree is a bit "red" right now (see the red bots here: > https://codereview.chromium.org/1160803002/). > > Other info: we also have "trybots" which, if you have trybot access, can help > test LLVM changes before they land (e.g., on Linux, Mac and Windows to help > cover machine types that you might not have access to). See > native_client/pnacl/try_git_change.py "Example usage" section. This patch looks > pretty safe, but for bigger patches that trybot script could be useful. > > Otherwise, this patch LGTM. Ok. Thanks. I've updated the description and will plan to commit the patches in the order you recommended. I will test with this single patch to see that it can stand on it's own.
What next? i don't think i have trybot access. I have not requested this. Not sure how to do that. We have our own build bots internally and once I understand this better we will set all that up so I can do testing internally before trying to push upstream. reed On 2015/05/29 20:26:20, reedkotler wrote: > On 2015/05/28 00:10:59, jvoung wrote: > > Could you update the change list description on this > http://codereview.chromium.org > > page? Git will take that description for its log entry, so it's good to get it > > right before landing =) > > > > > > The usual style is: > > > > ======= BEGIN template ==== > > one-liner description -- same as the "subject" in codereview > > > > more lines of description as needed. > > another line of description. > > more stuff. > > > > BUG= https://code.google.com/p/nativeclient/issues/detail?id=4167 > > ======= END template ==== > > > > The main thing is update the "BUG= ..." to the link. > > > > Otherwise, for this patch, I think a more specific one-liner could be more > > useful. E.g., the one-liner could be something like "Add mips32 to LLVM > > configure's list of allowed Subzero targets"? > > > > For ordering the patches, I'm not sure if you thought about it much, but I > think > > this LLVM repo patch could probably go first. It won't take effect until the > > NaCl repo patch lands so it's pretty safe. The NaCl repo change needs to land > > *after* the Subzero repo patch (otherwise Subzero won't know how to build the > > new target that you enabled via configure / CMake). > > > > Also, we are in the middle of debugging some LLVM 3.7 merge bugs, so the LLVM > > tree is a bit "red" right now (see the red bots here: > > https://codereview.chromium.org/1160803002/). > > > > Other info: we also have "trybots" which, if you have trybot access, can help > > test LLVM changes before they land (e.g., on Linux, Mac and Windows to help > > cover machine types that you might not have access to). See > > native_client/pnacl/try_git_change.py "Example usage" section. This patch > looks > > pretty safe, but for bigger patches that trybot script could be useful. > > > > Otherwise, this patch LGTM. > > Ok. Thanks. > > I've updated the description and will plan to commit the patches in the order > you recommended. > I will test with this single patch to see that it can stand on it's own.
I have checked that this patch by itself when applied still allows the toolchain build without errors. toolchain_build/toolchain_build_pnacl.py --verbose --clobber --install toolchain/linux_x86/pnacl_newlib_raw On 2015/06/02 01:37:33, reedkotler wrote: > What next? > > i don't think i have trybot access. I have not requested this. Not sure how to > do that. > > We have our own build bots internally and once I understand this better we will > set all that > up so I can do testing internally before trying to push upstream. > > reed > > On 2015/05/29 20:26:20, reedkotler wrote: > > On 2015/05/28 00:10:59, jvoung wrote: > > > Could you update the change list description on this > > http://codereview.chromium.org > > > page? Git will take that description for its log entry, so it's good to get > it > > > right before landing =) > > > > > > > > > The usual style is: > > > > > > ======= BEGIN template ==== > > > one-liner description -- same as the "subject" in codereview > > > > > > more lines of description as needed. > > > another line of description. > > > more stuff. > > > > > > BUG= https://code.google.com/p/nativeclient/issues/detail?id=4167 > > > ======= END template ==== > > > > > > The main thing is update the "BUG= ..." to the link. > > > > > > Otherwise, for this patch, I think a more specific one-liner could be more > > > useful. E.g., the one-liner could be something like "Add mips32 to LLVM > > > configure's list of allowed Subzero targets"? > > > > > > For ordering the patches, I'm not sure if you thought about it much, but I > > think > > > this LLVM repo patch could probably go first. It won't take effect until the > > > NaCl repo patch lands so it's pretty safe. The NaCl repo change needs to > land > > > *after* the Subzero repo patch (otherwise Subzero won't know how to build > the > > > new target that you enabled via configure / CMake). > > > > > > Also, we are in the middle of debugging some LLVM 3.7 merge bugs, so the > LLVM > > > tree is a bit "red" right now (see the red bots here: > > > https://codereview.chromium.org/1160803002/). > > > > > > Other info: we also have "trybots" which, if you have trybot access, can > help > > > test LLVM changes before they land (e.g., on Linux, Mac and Windows to help > > > cover machine types that you might not have access to). See > > > native_client/pnacl/try_git_change.py "Example usage" section. This patch > > looks > > > pretty safe, but for bigger patches that trybot script could be useful. > > > > > > Otherwise, this patch LGTM. > > > > Ok. Thanks. > > > > I've updated the description and will plan to commit the patches in the order > > you recommended. > > I will test with this single patch to see that it can stand on it's own.
On 2015/06/02 01:37:33, reedkotler wrote: > What next? I/Jim/???(petarj?) can help land this patch for now, until you get commit access. > i don't think i have trybot access. I have not requested this. Not sure how to > do that. > At some point we can request trybot access for you: https://www.chromium.org/getting-involved/become-a-committer#TOC-Try-job-access "It is very helpful to have already had some patches landed, but is not absolutely necessary." I don't know how they decide how many patches, but we can land these first couple patches first, and then if it seems useful later we can send out the request. (I assume for the gmail account?). > We have our own build bots internally and once I understand this better we will > set all that > up so I can do testing internally before trying to push upstream. Ok great! > > reed > > On 2015/05/29 20:26:20, reedkotler wrote: > > On 2015/05/28 00:10:59, jvoung wrote: > > > Could you update the change list description on this > > http://codereview.chromium.org > > > page? Git will take that description for its log entry, so it's good to get > it > > > right before landing =) > > > > > > > > > The usual style is: > > > > > > ======= BEGIN template ==== > > > one-liner description -- same as the "subject" in codereview > > > > > > more lines of description as needed. > > > another line of description. > > > more stuff. > > > > > > BUG= https://code.google.com/p/nativeclient/issues/detail?id=4167 > > > ======= END template ==== > > > > > > The main thing is update the "BUG= ..." to the link. > > > > > > Otherwise, for this patch, I think a more specific one-liner could be more > > > useful. E.g., the one-liner could be something like "Add mips32 to LLVM > > > configure's list of allowed Subzero targets"? > > > > > > For ordering the patches, I'm not sure if you thought about it much, but I > > think > > > this LLVM repo patch could probably go first. It won't take effect until the > > > NaCl repo patch lands so it's pretty safe. The NaCl repo change needs to > land > > > *after* the Subzero repo patch (otherwise Subzero won't know how to build > the > > > new target that you enabled via configure / CMake). > > > > > > Also, we are in the middle of debugging some LLVM 3.7 merge bugs, so the > LLVM > > > tree is a bit "red" right now (see the red bots here: > > > https://codereview.chromium.org/1160803002/). > > > > > > Other info: we also have "trybots" which, if you have trybot access, can > help > > > test LLVM changes before they land (e.g., on Linux, Mac and Windows to help > > > cover machine types that you might not have access to). See > > > native_client/pnacl/try_git_change.py "Example usage" section. This patch > > looks > > > pretty safe, but for bigger patches that trybot script could be useful. > > > > > > Otherwise, this patch LGTM. > > > > Ok. Thanks. > > > > I've updated the description and will plan to commit the patches in the order > > you recommended. > > I will test with this single patch to see that it can stand on it's own.
Sorry I just remembered this, but have you signed the CLA? https://www.chromium.org/developers/contributing-code/external-contributor-ch... "If there's a corporate CLA for the author's company, it must list the person explicitly (or the list of authorized contributors must say something like "All employees")." I could see Imagination Technologies Ltd listed, but I can't tell if it's "All employees". Otherwise I don't see your @gmail.com account listed, so wanted to check.
On 2015/06/04 16:13:50, jvoung wrote: > Sorry I just remembered this, but have you signed the CLA? > > https://www.chromium.org/developers/contributing-code/external-contributor-ch... > > "If there's a corporate CLA for the author's company, it must list the person > explicitly (or the list of authorized contributors must say something like "All > employees")." > > I could see Imagination Technologies Ltd listed, but I can't tell if it's "All > employees". Otherwise I don't see your @gmail.com account listed, so wanted to > check. there is now a reed.kotler@imgtec.com now that has a CLA. how to I change patches that are in progress to this new account? not sure how I proceed from here. tia. reed
On 2015/06/05 20:46:30, reed.kotler wrote: > On 2015/06/04 16:13:50, jvoung wrote: > > Sorry I just remembered this, but have you signed the CLA? > > > > > https://www.chromium.org/developers/contributing-code/external-contributor-ch... > > > > "If there's a corporate CLA for the author's company, it must list the person > > explicitly (or the list of authorized contributors must say something like > "All > > employees")." > > > > I could see Imagination Technologies Ltd listed, but I can't tell if it's "All > > employees". Otherwise I don't see your @gmail.com account listed, so wanted to > > check. > > there is now a mailto:reed.kotler@imgtec.com now that has a CLA. > > how to I change patches that are in progress to this new account? > > not sure how I proceed from here. > > tia. > > reed AFAIK, there is no way to "hijack" someone else's CL. You probably have to just create a new CL and close the old CL. Also document the weirdness by adding comments in the old and new CLs pointing to each other.
On 2015/06/05 22:31:39, stichnot wrote: > On 2015/06/05 20:46:30, reed.kotler wrote: > > On 2015/06/04 16:13:50, jvoung wrote: > > > Sorry I just remembered this, but have you signed the CLA? > > > > > > > > > https://www.chromium.org/developers/contributing-code/external-contributor-ch... > > > > > > "If there's a corporate CLA for the author's company, it must list the > person > > > explicitly (or the list of authorized contributors must say something like > > "All > > > employees")." > > > > > > I could see Imagination Technologies Ltd listed, but I can't tell if it's > "All > > > employees". Otherwise I don't see your @gmail.com account listed, so wanted > to > > > check. > > > > there is now a mailto:reed.kotler@imgtec.com now that has a CLA. > > > > how to I change patches that are in progress to this new account? > > > > not sure how I proceed from here. > > > > tia. > > > > reed > > AFAIK, there is no way to "hijack" someone else's CL. You probably have to just > create a new CL and close the old CL. Also document the weirdness by adding > comments in the old and new CLs pointing to each other. How do I change the user that "git cl upload" submits under? Tia. Reed
On 2015/06/06 00:45:25, reed.kotler wrote: > On 2015/06/05 22:31:39, stichnot wrote: > > On 2015/06/05 20:46:30, reed.kotler wrote: > > > On 2015/06/04 16:13:50, jvoung wrote: > > > > Sorry I just remembered this, but have you signed the CLA? > > > > > > > > > > > > > > https://www.chromium.org/developers/contributing-code/external-contributor-ch... > > > > > > > > "If there's a corporate CLA for the author's company, it must list the > > person > > > > explicitly (or the list of authorized contributors must say something like > > > "All > > > > employees")." > > > > > > > > I could see Imagination Technologies Ltd listed, but I can't tell if it's > > "All > > > > employees". Otherwise I don't see your @gmail.com account listed, so > wanted > > to > > > > check. > > > > > > there is now a mailto:reed.kotler@imgtec.com now that has a CLA. > > > > > > how to I change patches that are in progress to this new account? > > > > > > not sure how I proceed from here. > > > > > > tia. > > > > > > reed > > > > AFAIK, there is no way to "hijack" someone else's CL. You probably have to > just > > create a new CL and close the old CL. Also document the weirdness by adding > > comments in the old and new CLs pointing to each other. > > How do I change the user that "git cl upload" submits under? > > Tia. > > Reed Can you try """ To login with a different email run: depot-tools-auth login https://codereview.chromium.org To logout and purge the authentication token run: depot-tools-auth logout https://codereview.chromium.org """ Excerpt from: https://groups.google.com/a/chromium.org/d/msg/chromium-dev/K9cGObPZyyk/om-ct... Thanks!
Message was sent while issue was closed.
On 2015/06/06 16:51:30, jvoung wrote: > On 2015/06/06 00:45:25, reed.kotler wrote: > > On 2015/06/05 22:31:39, stichnot wrote: > > > On 2015/06/05 20:46:30, reed.kotler wrote: > > > > On 2015/06/04 16:13:50, jvoung wrote: > > > > > Sorry I just remembered this, but have you signed the CLA? > > > > > > > > > > > > > > > > > > > > https://www.chromium.org/developers/contributing-code/external-contributor-ch... > > > > > > > > > > "If there's a corporate CLA for the author's company, it must list the > > > person > > > > > explicitly (or the list of authorized contributors must say something > like > > > > "All > > > > > employees")." > > > > > > > > > > I could see Imagination Technologies Ltd listed, but I can't tell if > it's > > > "All > > > > > employees". Otherwise I don't see your @gmail.com account listed, so > > wanted > > > to > > > > > check. > > > > > > > > there is now a mailto:reed.kotler@imgtec.com now that has a CLA. > > > > > > > > how to I change patches that are in progress to this new account? > > > > > > > > not sure how I proceed from here. > > > > > > > > tia. > > > > > > > > reed > > > > > > AFAIK, there is no way to "hijack" someone else's CL. You probably have to > > just > > > create a new CL and close the old CL. Also document the weirdness by adding > > > comments in the old and new CLs pointing to each other. > > > > How do I change the user that "git cl upload" submits under? > > > > Tia. > > > > Reed > > Can you try > > """ > To login with a different email run: > depot-tools-auth login https://codereview.chromium.org > To logout and purge the authentication token run: > depot-tools-auth logout https://codereview.chromium.org > """ > > Excerpt from: > https://groups.google.com/a/chromium.org/d/msg/chromium-dev/K9cGObPZyyk/om-ct... > > Thanks! This issue moved to: https://codereview.chromium.org/1163713009/ |
