1363666 - stylo: Allow llvm 4.0 for stylo

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, enhancement)

Description

[Emilio Cobos Álvarez (:emilio)]

The latest bindgen update fixed the bugs that prevented it from being used, AFAICT.

Comment 1

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1363666: Remove more LLVM version requirements for stylo.

Review commit: https://reviewboard.mozilla.org/r/137888/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/137888/

Comment 2

[Jan Beich]

Comment on attachment 8866267 [details]
Bug 1363666: Remove more LLVM version requirements for stylo.

https://reviewboard.mozilla.org/r/137888/#review141024

::: toolkit/moz.configure:622
(Diff revision 1)
>  
>  @imports(_from='textwrap', _import='dedent')
>  def check_minimum_llvm_config_version(llvm_config):
>      version = Version(invoke_llvm_config(llvm_config, '--version'))
>      min_version = Version('3.9.0')
>      # For various reasons, bindgen doesn't work with LLVM 4.0.x.

Is this comment still necessary?

Comment 3

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8866267 [details]
Bug 1363666: Remove more LLVM version requirements for stylo.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/137888/diff/1-2/

Comment 4

[Emilio Cobos Álvarez (:emilio)]

(In reply to Jan Beich from comment #2)
> Comment on attachment 8866267 [details]
> Bug 1363666: Remove more LLVM version requirements for stylo.
> 
> https://reviewboard.mozilla.org/r/137888/#review141024
> 
> ::: toolkit/moz.configure:622
> (Diff revision 1)
> >  
> >  @imports(_from='textwrap', _import='dedent')
> >  def check_minimum_llvm_config_version(llvm_config):
> >      version = Version(invoke_llvm_config(llvm_config, '--version'))
> >      min_version = Version('3.9.0')
> >      # For various reasons, bindgen doesn't work with LLVM 4.0.x.
> 
> Is this comment still necessary?

It is not, good catch.

Comment 5

[Nathan Froyd [:froydnj]]

Comment on attachment 8866267 [details]
Bug 1363666: Remove more LLVM version requirements for stylo.

https://reviewboard.mozilla.org/r/137888/#review141152

rs=me, assuming that bindgen DTRT.

::: toolkit/moz.configure:626
(Diff revision 2)
> -    if version < min_version or (version >= Version('4.0.0') and
> -                                 version < Version('5.0.0')):
>          die(dedent('''\
>          llvm installation {} is incompatible with Stylo bindgen.
>  
> -        To compile Stylo, please install version {} of
> +        To compile Stylo, please install version {}+ of

Maybe say "version {} or higher..." here for clarity.

Comment 6

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8866267 [details]
Bug 1363666: Remove more LLVM version requirements for stylo.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/137888/diff/2-3/

Comment 7

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8866267 [details]
Bug 1363666: Remove more LLVM version requirements for stylo.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/137888/diff/3-4/

Comment 8

[Mozilla Autoland]

hg error in cmd: hg rebase -s 43da2435228b -d 3b02ca42da3a: abort: can't rebase public changeset 43da2435228b
(see 'hg help phases' for details)

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ff09d3c3180d
Remove more LLVM version requirements for stylo. r=froydnj

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/ff09d3c3180d

Comment 11

[Xidorn Quan [:xidorn] UTC+10]

It seems LLVM 4.0 works on Windows build as well. I guess we can start moving people to use 4.0 by default. If all people can move to 4.0, we can remove workarounds for old version like nsTArrayBorrowed.

Comment 12

[Jan Beich]

Comment on attachment 8866267 [details]
Bug 1363666: Remove more LLVM version requirements for stylo.

https://reviewboard.mozilla.org/r/137888/#review141474

::: toolkit/moz.configure:622
(Diff revisions 2 - 3)
>  
>  @imports(_from='textwrap', _import='dedent')
>  def check_minimum_llvm_config_version(llvm_config):
>      version = Version(invoke_llvm_config(llvm_config, '--version'))
>      min_version = Version('3.9.0')
> +    # For various reasons, bindgen doesn't work with LLVM 4.0.x.

Can you remove the comment again?

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7986e8790db6
followup - Remove outdated comment. r=me

Comment 14

[Emilio Cobos Álvarez (:emilio)]

(In reply to Jan Beich from comment #12)
> Comment on attachment 8866267 [details]
> Bug 1363666: Remove more LLVM version requirements for stylo.
> 
> https://reviewboard.mozilla.org/r/137888/#review141474
> 
> ::: toolkit/moz.configure:622
> (Diff revisions 2 - 3)
> >  
> >  @imports(_from='textwrap', _import='dedent')
> >  def check_minimum_llvm_config_version(llvm_config):
> >      version = Version(invoke_llvm_config(llvm_config, '--version'))
> >      min_version = Version('3.9.0')
> > +    # For various reasons, bindgen doesn't work with LLVM 4.0.x.
> 
> Can you remove the comment again?

Gah, that's the patch mozreview couldn't rebase. Done, thanks again

Comment 15

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/7986e8790db6