Created attachment 274420 [details] [review]
giscanner: Improve Visual C++ compiler detection

Hi,

This is my patch at tackling the issue, by moving the compiler detection items into a function in giscanner/utils.py:

-On Windows, checking the presence of cl.exe in the PATH,
 by running "cl.exe /?" (The Visual C++ compiler command).
 If it is indeed there, check its version string, that is
 interestingly output via stderr rather than stdout; and
 if the version string check succeeded, use cl.exe as the
 compiler and linker commands.  Otherwise, query CC, as we
 did before, for the compiler and linker commands.

-On other systems, query CC for the compiler and linker
 commands as we did previously.

Please note: The patch here is built on top of my patches in bug 728190, in which the patches in that bug, and consequently this bug, has been checked on a Fedora 20 system using 'make distcheck' successfully.

With blessings, thank you!

Created attachment 274452 [details] [review]
giscanner: Improve Visual C++ compiler detection (take ii)

Hi,

Sorry, my previous patch forgot to account for the case when the version string is not the one that is expected from cl.exe.

This is still built upon the patches in the bug in the last comment, and is checked successfully with 'make distcheck' on Fedora 20.

With blessings, thank you!

Review of attachment 274452 [details] [review]:

Can we avoid the utils dumping ground and put get_compiler_cmds() into a new file, something like "ccompiler.py"? ccompiler.py can then grow into module containing a compiler interface that abstracts away the need to have lines like "if 'cl.exe' in cpp_args" scattered throughout the code base. 

Similarly, I think it would be a good idea to look into using the distutils compiler interface which might already do everything we need:
https://docs.python.org/2/distutils/apiref.html#module-distutils.ccompiler

::: giscanner/utils.py
@@ +213,3 @@
+
+def get_compiler_cmds():
+    if os.name == 'nt':

Would this cause problems in an environment like MinGW? e.g. if os.name is still 'nt' in a MinGW environment with CC set, the developer might expect CC to be used, whereas this would potentially pickup the MS compiler instead? If this is the case, perhaps the "CC" environment var should be checked for existence first, then the 'nt' os and cl.exe compiler check as a fallback?

(I'm a out of my league here but thought it might be a possibility).

Some additional notes on the possibility of using distutils.ccompiler:

distutils.ccompiler.new_compiler is a factory for creating a compiler instance with platform customization. Depending on what we need this might not be usable.

distutils.sysconfig.customize_compiler(compiler) needs to be called on "unix" compiler instances to pickup the "CC" environment var and related vars (CFLAGS, LDFLAGS, etc...).

If we want to switch g-ir-scanner to use cl.exe for pre-processing [1], a sub-class of distutils.msvc9compiler.MSVCCompiler would need to be created which implements the "preprocess" method. I verified cl.exe supports flags to generate a preprocessed file (/E, /P or /EP) so it seems possible. With the caveat that it doesn't support reading from stdin (which I don't see as a big deal because we could use a temp file). cl.exe supports writing the preprocessed output to stdout (/E and /EP), or a file (/P) with the same name as the input file but with a ".i" extension.

[1] https://git.gnome.org/browse/gobject-introspection/tree/giscanner/sourcescanner.py?id=GOBJECT_INTROSPECTION_1_40_0#n285

Created attachment 275968 [details] [review]
giscanner: Add an abstraction for compiler-specific items

Hello Simon,

OK, here's my patch to try to abstract the compiler-specific parts... sans the part on using distutils.ccompiler due to the following...

(In reply to comment #4)
> Some additional notes on the possibility of using distutils.ccompiler:

I would need some help here, as:
-It seems to me distutils.ccompiler is mainly used
 for building Python C/C++ modules, so I am not too sure
 how this comes into play in our situtation.  Need to
 investigate more on this, as I am definitely not an expert
 on this area.
-The situation of Python on Windows is a bit special,
 as the Python 2.6/2.7 binaries as provided by www.python.org
 are built with Visual Studio 2008.  I'm not sure whether this
 will pose a problem if we are using MinGW to build g-i (I don't
 have MinGW/MSYS to check that).  As a side note, Python 3.3+
 is built with Visual Studio 2010 by default, and Python 3.0-3.2
 is also built with Visual Studio 2008 by default.

> If we want to switch g-ir-scanner to use cl.exe...

Yup, there's bug 728190 for this (there were some extra changes needed though), which my patches for this bug were originally built upon.

I will post another patchset for the changes in the other scripts in giscanner in the next day or two in regards to this.

With blessings, thank you for your feedback!

Review of attachment 275968 [details] [review]:

This is a good start. I think some of the logic could be reworked for clarity by breaking it into finer grained classes and functions. This will also help unit-testing and readability. Basically I think we could break it down as follows (untested sudo Python):

def find_compiler_cmd(env):
    """Returns a list of strings representing a valid compiler.

    Checks for CC in the above environment and attempts defaulting to "cc"
    with a fallback to "cl.exe" on Windows.

    Raises SystemExit if a compiler is not found.
    """
    ... I think Windows always requires .exe even with cc?
    if os.name == 'nt':
        exeext = '.exe'
    else:
        exeext = ''

    compiler_cmd = env.get('CC', None)

    # If CC is set, trust the user knows what they are doing and don't do any validation.
    # because we cannot assume what the compiler actually is.
    if compiler_cmd is not None:
        return compiler_cmd.split()

    # Attempt using cc as default (even on Windows)
    compiler_cmd = ['cc' + exeext]
    try:
        output = subprocess.check_output(compiler_cmd + ['-v'])
        ... Validate output for cc?
        return compiler_cmd
    except CalledProcessError:
        pass

    # Attempt fallback to cl.exe on Windows
    if os.name == 'nt':
        compiler_cmd = ['cl' + exeext]
        try:
            output = subprocess.check_output(compiler_cmd + ['/?'])
            # Validate output for msvc?
            return compiler_cmd
        except CalledProcessError:
            pass

    raise SystemError('A valid compiler could not be found, '
                      'try setting the CC environment variable.')


... now when setting up the Compiler object, we can dispatch a concrete compiler based on cl.exe existing in the compiler_cmd:

class CCompiler(object):
    # common shared functionality and overall API description.

class MSVCCompiler(CCompiler):
    # cl.exe specific overrides and functionality

class UnixCompiler(CCompiler):
    # gcc specific overrides and functionality

def create_compiler(env):
    compiler_cmd = find_compiler_cmd(os.environ)
    if 'cl.exe' in compiler_cmd:
        return MSVCCompiler(compiler_cmd, env)
    else:
        return UnixCompiler(compiler_cmd, env)

::: giscanner/ccompiler.py
@@ +41,3 @@
+        do_extra_check = True
+        if os.name != 'nt' or \
+           (compiler_cmd_env not in 'cl' and \

These checks look backwards. Should be ('cl' not in compiler_cmd_env) also note that will match 'clang' so don't we want to get rid of 'cl' and only use 'cl.exe' ?

@@ +56,3 @@
+                                        stdout=subprocess.PIPE,
+                                        stderr=subprocess.PIPE)
+                gcc_out, gcc_verinfo = proc.communicate()

Probably simpler to use:
    output = subprocess.check_output(...)

@@ +58,3 @@
+                gcc_out, gcc_verinfo = proc.communicate()
+                lines = gcc_verinfo.splitlines()
+                for line in lines:

No need for the temporary:
    for line in output.splitlines()

@@ +59,3 @@
+                lines = gcc_verinfo.splitlines()
+                for line in lines:
+                    if sys.version_info >= (3, 0):

I wouldn't worry about Python 3 until the patches in bug 679438 land. The approach over there enables unicode literals and then always encode/decode when dealing with subprocess input/output, making the code portable to Python 2 and 3.

@@ +66,3 @@
+                        if 'mingw32' in l:
+                            do_extra_check = False
+            except:

Bare except or Exception will mask programming errors in the above logic, use:
    except CalledProcessError:

@@ +91,3 @@
+                else:
+                    l = lines[0]
+                if l.startswith(check_ver_string_1) or \

startswith accepts a tuple, so this can be:
    if l.startswith((check_ver_string_1, check_ver_string_2)):

@@ +104,3 @@
+
+    def _set_compiler_flags(self):
+        if self.is_msvc is True:

I don't think identity tests are ok for truth values. Simply use:
    if self.is_msvc:
        ...

Review of attachment 275968 [details] [review]:

::: giscanner/ccompiler.py
@@ +66,3 @@
+                        if 'mingw32' in l:
+                            do_extra_check = False
+            except:

This should actually be:
    except (OSError, subprocess.CalledProcessError):

Created attachment 276056 [details] [review]
giscanner: Add a ccompiler module (which uses distutils)

Hi,

In today's hacking, it seems that I could get distutils on acquiring the compiler (as suggested by Simon) working, so I have made another version of ccompiler, though using distutils more broadly is going to need more investigation and research.  

However, I do need some help here, although this (together with my following patches in this bug) does make it through 'make distcheck'[1] on my Fedora 20 system), namely:
-Whether this does not break the build on other systems that g-i
 supports, as I don't have access to them.
-For LRN especially, and/or for people that are more knowledgeable
 in autotools, how the autotools scripts can be used to pass in
 --compiler to g-ir-scanner, etc.[2]

Please let me know whether this, or the previous patch, is preferred.

With blessings, thank you!

[1]: That is after updating the Regress-1.0-expected.gir, as Ryan did some work on introspection very recently.

[2]: The official Python binaries are built with Visual Studio, so using distutils to acquire the default compiler would not work there, so instead one needs to pass in a --compiler=mingw32 for MinGW32 builds to work.

Created attachment 276057 [details] [review]
Updates to giscanner/scannerlexer.l to support using the MSVC preprocessor

Hi,

This basically shifts the patch on scannerlexer.l to this bug from bug 728190.

With blessings, thank you!

Created attachment 276058 [details] [review]
giscanner: Update to use the ccompiler module

Hi,

This updates the various giscanner Python scripts to make use of the ccompiler module (the second version), to acquire compiler-specific items and when applicable, do compiler-specific work.  It also enables some clean-ups in those scripts, especially as we now pass in the names of the .lib files to resolve from them the DLLs that the introspection files need to refer to.

This also incorporates the updates from bug 728190 to enable Visual C++ builds to not depend on a GCC/MinGW installation to preprocess the headers/sources during the scanning stage.

With blessings, thank you!

About distutils: i strongly distrust it. There was a long discussion about building CPython with MinGW GCC. This required a number of patches, and changes to distutils (which were caught in a Windows == MSVC vs GNU == gcc dichotomy). Python maintainers were not amused. I don't know what the current state of this is at the moment. But it did not touch 2.7.x back then, and it is unlikely to do so now.

So i would not want to rely on distutils for this (i.e. it's fine to ask distutils, as long as i can override anything it detects).

There was a --compiler option mentioned. That would be OK, since we have autoconf macros and GI makefile templates, which do pass a lot of arguments. They can pass --compiler as well. Actually, right now they *shuould* be passing CC from what makefiles have defined; they don't, which is why i've had to patch many packages' makefiles to do that; this is a bug. So --compiler=$(CC) should be trivial to add. This should override any detection GI might do, as autotools usually knows better.

(In reply to comment #11)
> About distutils: i strongly distrust it. There was a long discussion about
> building CPython with MinGW GCC. This required a number of patches, and changes
> to distutils (which were caught in a Windows == MSVC vs GNU == gcc dichotomy).

Thanks for the input. It would be nice to have a reference to the discussion. Is the distrust limited to the factory for creating instances of the concrete compiler classes (distutils.ccompiler.new_compiler [1]) or the implementations of the classes themselves?

I think even if we could use distutils, we still need our own factory for creating the compiler instances. AFAICT distutils.ccompiler.new_compiler doesn't allow creation of custom compiler sub-classes which I think is needed for MSVC. This would also give us control over the default compiler choice.

In any event, rolling our own classes modeled after the distutils.ccompiler API (but probably much simpler) would be a good design choice (see comment #6). It would help remove all the various "if is_msvc" conditionals scattered around which is difficult to read and maintain.

> There was a --compiler option mentioned.

Would this be a flag to g-ir-scanner and related tools?

[1] https://docs.python.org/2/distutils/apiref.html#distutils.ccompiler.new_compiler

Hello Simon,

My personal take on the following items (sorry, could be a bit lengthy):

(In reply to comment #12)
> Is the distrust limited to the factory for creating instances of the concrete
> compiler classes (distutils.ccompiler.new_compiler [1]) or the implementations
> of the classes themselves?

When we try to use distutils to detect the default compiler on Windows (using get_default_compiler()), it would always acquire Visual C++ (AFAICT, on Windows we are using the Python official distribution from www.python.org for g-i).

One thing that hit me in the course is that if I try to acquire the compiler command via distutils for Visual C++ (please see patch in comment 8), I would always get the Visual C++ 2008 compiler command (and environment) from it, for Python 2.7.x, which would not be good.  (The same issue, I think, is likely to hit when we have g-i usable under Python 3.3+, in which the default compiler is MSVC 2010).

The reason is, we may be using Visual C++ 2010 (for example) to build the introspection files during the build of g-i using MSVC 2010, and having a MSVC 2008 environment would mean that we would try to resolve the .lib's that correspond to that environment, and would therefore mean that the introspection files would link to the MSVC 2008 DLLs instead of the MSVC 2010 DLLs, which is not desirable.

Probably LRN has more on this, though.

> would help remove all the various "if is_msvc" conditionals scattered around
> which is difficult to read and maintain.

Quick question: Would having a is_msvc tag in the class (that is set during the init of the class that could be referred to quickly) I had in my patches be alright?

> Would this be a flag to g-ir-scanner and related tools?

Yup, please see the patch in comment 10.  I do, however, as in the other parts of the scripts, use the MSYSTEM env var under Windows, so that we can try to detect MinGW usage by default even if a compiler option is not supplied.  AFAIK, the MinGW builds of any of the GNOME items are always done under MSYS, if done on Windows.  The (optional) compiler option is strictly used only on Windows at this time.

---
LRN: Can you do me a favor, can you please run the build of g-i with my patches, to see whether the compiler detection would work for you?  Greatly appreciated!
---

With blessings, thank you!

Apart from a typo in attachment 275968 [details] [review] ('line', not 'l'), patches apply (minus the attachment 276056 [details] [review]; i imagine it and 275968 are mutually exclusive).

They also seem to be working. At least GI itself builds.

Some improvements that can be done:
1) Don't feed GCC from stdin, use a tempfile (same as with MSVC).
2) Don't read compiler output from its stdout, make it output into a tempfile and then read that (see [1]).

[1] https://mail.gnome.org/archives/gtk-list/2014-April/msg00020.html

Hello LRN,

(In reply to comment #14)
>i imagine it and 275968 are mutually exclusive).
Would using attachment 276056 [details] [review] in place of 275968 work for you, even without using --compiler?

> They also seem to be working. At least GI itself builds.
>

Nice to know this :)

> Some improvements that can be done:
> 1) Don't feed GCC from stdin, use a tempfile (same as with MSVC).
> 2) Don't read compiler output from its stdout, make it output into a tempfile
> and then read that (see [1]).

I'll look into these-for (1), do people think we want to do that for all cases, or for Windows only?

Thanks a bunch, with blessings.

(In reply to comment #15)
> (In reply to comment #14)
> >i imagine it and 275968 are mutually exclusive).
> Would using attachment 276056 [details] [review] in place of 275968 work for you, even without using --compiler?

I was not using it.

Actually, i was confused about attachment 275968 [details] [review] and attachment 276056 [details] [review].

What i did test was 276056.

With 275968 the patches apply, but GI fails to compile:
Traceback (most recent call last):

+ Trace 233588

(In reply to comment #13)
> Quick question: Would having a is_msvc tag in the class (that is set during the
> init of the class that could be referred to quickly) I had in my patches be
> alright?

I think in the short term it is fine. But eventually we should only have a single "is msvc" condition in a factory function that either creates an MSVCCompiler or UnixCompiler object.

Hello LRN,

(In reply to comment #16)
> Actually, i was confused about attachment 275968 [details] [review] and attachment 276056 [details] [review].
> 
> What i did test was 276056.

Sorry, I should have been more clear about my requests, as it was mainly about 276056 that I needed more insight... But great to know that 276056 works for you :)

> 
> With 275968 the patches apply, but GI fails to compile:
> Traceback (most recent call last):

276058 is done to work with 276056, hence the problems :P.  Sorry about this, but thanks a bunch anyways!

---
Hello Simon,

> I think in the short term it is fine. But eventually we should only have a
> single "is msvc" condition in a factory function that either creates an
> MSVCCompiler or UnixCompiler object.

I see, I'll look into that.

Thanks for the input and feedback.  With blessings, thank you!

Hi,

Sorry for not being able to revisit the issue lately...

It does seem, that as we want to support different Visual Studio builds (i.e. 2008/2010/2012/2013), totally relying on Python's ccompiler class would not be a good option, as the msvccompiler class will override the current compiler settings with the one that is used to build Python (2008 for Python 2.7.x), unless some hacky workarounds are used to circumvent that, along with resetting the PATH variable during the build to acquire the compiler details, which would break the build unless we use a workaround to save the PATH and re-apply it after the introspection binary is built (hacky in my opinion too).

This means, if I try to build the introspection files with Visual Studio 2010 32 bit, even when I try to set DISTUTILS_USE_SDK, the 2008 compiler and linker will be called, and the .gir files will then link to the 2008-built DLLs, such as glib-2-vs9.dll instead of glib-2-vs10.dll.

Any other suggestions on this part would be welcome, but probably this is as far as I could go on the compiler factory part.

I would still look into LRN's suggestion on using a temp file for the builds of the introspection files-please let me know if this is for Windows, or for all builds.

With blessings, thank you!

Hi,

(In reply to comment #19)

> It does seem, that as we want to support different Visual Studio builds (i.e.
> 2008/2010/2012/2013), totally relying on Python's ccompiler class would not be
> a good option...

It seems that I was wrong about this, so far, as I could do it in the NMake Makefiles to trick distutils that we are always using a Windows SDK compiler, so that it would not try to launch a new vsvars environment (i.e. a new MSVC compiler environment), which was the source to this grievance.  However, there is one catch...

(In reply to comment #4)
> If we want to switch g-ir-scanner to use cl.exe for pre-processing [1], a
> sub-class of distutils.msvc9compiler.MSVCCompiler would need to be created
> which implements the "preprocess" method.

Unfortunately, the MSVC compiler classes seem not to have implemented the preprocessor method (only the UNIX and bcc(Borland) classes have it, AFAICT, at least in Python 2.7.6), so it seems that we might be better off sticking to the original method that is used in sourcescanner._parse().

With blessings, thank you!

> (In reply to comment #4)
> > If we want to switch g-ir-scanner to use cl.exe for pre-processing [1], a
> > sub-class of distutils.msvc9compiler.MSVCCompiler would need to be created
> > which implements the "preprocess" method.
> 
> Unfortunately, the MSVC compiler classes seem not to have implemented the
> preprocessor method (only the UNIX and bcc(Borland) classes have it, AFAICT, at
> least in Python 2.7.6), so it seems that we might be better off sticking to the
> original method that is used in sourcescanner._parse().

That's what I was stating. If this is the only thing holding back usage of distutils, then it is a matter of sub-classing MSVCCompiler and implementing the preprocess method ourselves:

    class MSVCCompiler(distutils.msvc9compiler.MSVCCompiler):
        def preprocess(self, ...):
            ...

Then we would need to implement our own create_compiler() factory to create the customized instances.

To be clear with regards to my previous reviews/comments, the main point is that I think we should be putting compiler related functions (especially new ones) in a new file named ccompiler.py not utils.py. I don't expect a re-architecture in one go, but rather a re-factoring strategy which moves relevant bits (unchanged as much as possible) into functions or classes in ccompiler.py. These bits can eventually be combined into a class hierarchy modeled after the distutils ccompiler classes. If we can use distutils or parts of it, then I think that would be an extra bonus. But it probably makes sense to move everything compiler related into our own ccompiler.py file first.

Briefly reviewing the patches, I think they will need to be broken up a bit in terms of code movement into ccompiler.py. I will comment on the patches individually. Thanks for your work so far.

Created attachment 280247 [details] [review]
Add a CCompiler module that makes use of distutils (take iii)

Hello Simon,

This is my progress so far, some of which may or may not be ready for committing into master.

This version of ccompiler.py tries to make more use of distutils...

Created attachment 280248 [details] [review]
giscanner/dumper.py: Make use of the CCompiler module

Hi,

This updates dumper.py so that the compilation and linking of the introspection program can be done with distutils.ccompiler via the new ccompiler module...

Created attachment 280251 [details] [review]
shlibs.py: Windows: Use CCompiler module to deduce DLL from library

Hi,

This basically uses the shlib resolution for Windows in ccompiler.py, which was moved there.

(In reply to comment #22)
> This is my progress so far, some of which may or may not be ready for
> committing into master.
> 
> This version of ccompiler.py tries to make more use of distutils...

Oh, probably I should mention that this is still [wip], as the preprocessing part still needs to be sorted out.

Created attachment 280252 [details] [review]
giscanner/scannermain.py: Add a --compiler option

Hi,

This adds a --compiler option so that it can be used on Windows to tell giscanner which compiler is being used, if needed, such as when using MinGW to build the introspection files without using MSYS, for instance.  CCompiler already has the capabilities to guess the compiler used on Windows by judging whether we are running the introspection build in MSYS...

Created attachment 280253 [details] [review]
[strictly wip]: Don't use stdin/stdout in preprocessing

Hi,

This is a strictly-wip, not for committing, patch for having sourcescanner.py to not use stdin to feed the preprocessor, and not use stdout to store the results of the preprocessing.

As Simon mentioned earlier, more investigation needs to be in for using distutils for preprocessing, as Python's msvccompiler modules do not support preprocessing out-of-the box.

This is what I have so far.

With blessings, thank you!

Created attachment 280468 [details] [review]
giscanner: Add a ccompiler module that uses distutils (v4)

Hi,

I have updated the ccompiler module that I was working on so that it would both work under Windows, and pass distcheck in Linux, and attempt to make more use of distutils.  Still yet to look at the preprocessing part though, which at this point still work in the former way...

Created attachment 280469 [details] [review]
giscanner/dumper.py: Use the new ccompiler module (v3)

Hi,

This updates dumper.py accordingly, and also does a minor fix on the executable extension when there is none, using '' instead of using Python's default None, so it would work as before...

Created attachment 280470 [details] [review]
[wip]: giscanner/sourcescanner.py: Don't use stdin to feed the preprocessor, and output preprocessor results to file

Hi,

This updates sourcescanner accordingly, due to the updates in ccompiler.py, like the previous version of the patch, and fixes the run on *NIX.

With blessings, thank you!

Created attachment 280696 [details] [review]
giscanner: Add modules for preprocessing and building introspection programs using distutils

Hi,

This adds two modules for using distutils to preprocess the sources for introspection, and compile and linking the introspection program, where one module calls the machinery in distutils for these jobs, and the other provides an implementation of preprocess() for MSVC builds, which the stock distutils do not provide.

This, as a result, would make giscanner more portable across platforms that the GTK+ stack supports...

Created attachment 280699 [details] [review]
giscanner/scannermain.py: Add a --compiler option[v2]

Hi,

This makes the --compiler option change on giscanner/scannermain.py be a bit cleaner, as creating a ccompiler object isn't really necessary here...

Created attachment 280700 [details] [review]
sourcescanner: Use distutils for preprocessing the sources

Hi,

This updates giscanner/sourcescanner.py to:

-Use temp files for the inputs and outputs for the preprocessing, as:
--MSVC does not support stdin as input to the preprocessor
--MSYS could have trouble dealing with line endings from stding/stdout

-Use distutils.ccompiler.preprocess() to run the preprocessor, which
 would make giscanner more portable in terms of the supported platforms
 of Python and the GTK+ stack.

So, this is what I have for this bug.

With blessings, thank you!

Created attachment 280777 [details] [review]
giscanner: Add modules for preprocessing and building introspection programs using distutils (take ii)

Hi,

This patch fixes PEP-8 issues caught by 'make check' in the previous version of this patch.  The current patches both pass 'make distcheck' on my Fedora 20 x64 system, and allows building of introspection files on Visual C++ without the need to use the GCC preprocessor.

Since I have no access to other platforms that g-i supports (including an MSYS/MinGW build environment), any reports on this is highly appreciated.

With blessings, thank you!

Which of the patches should be applied on top of current GI git master? In which order?

Hello LRN,

(In reply to comment #34)
> Which of the patches should be applied on top of current GI git master? In
> which order?

You will need all patches, or at least everything besides the scannerlexer.l patch for MinGW builds.

The patches should be  applicable in any order, but for references:
-giscanner: add modules for preprocessing...
-giscanner/scannermain.py: Add a compiler option...
-giscanner/dumper.py: Use the new ccompiler module...
-sourcescanner: Use distutils for preprocessing...
-shlibs.py: Windows use ccompiler module to deduce...
-Updates to giscanner/scannerlexer.l...

Thanks for helping out to test this, greatly appreciated.

With blessings, thank you!

Created attachment 280968 [details] [review]
Fixes preprocessor being None for mingw32 compiler

Created attachment 280969 [details] [review]
Fixes a typo in ccompiler.py

Applied patches in following order:

giscanner-Add-Modules-Using-distutils
Add-a-compiler-option
giscanner-dumper.py-Use-the-New-CCompiler-Module
sourcescanner.py-Use-distutils-for-preprocessing
giscanner-shlibs.py-Use-CCompiler-Module
giscanner-scannerlexer.l-Update-to-Support-MSVC-Prep

Encountered an error:

Traceback (most recent call last):

+ Trace 233834

Makefile:3395: recipe for target 'GLib-2.0.gir' failed

Fixed it (attachment 280968 [details] [review]).

Found a typo. Fixed it (attachment 280969 [details] [review]).

Now compiled scanner fails to execute, because it can't find msvcr90.dll.

My guess is that msvcr90 comes from get_msvcr() in distutils/cygwincompiler.py.

I'm unsure whether it actually is necessary to link to msvcr90 (as opposed to msvcrt) for things to work - after all, we've been linking to msvcrt for years with no adverse effect on Python interaction (in fact, both Python and GI have special C code that ensures that CRT version mismatch doesn't cause problems - see pygi_source_scanner_parse_file()).

I don't feel comfortable hacking on distutils, and so far i haven't found a way to work around it.

Created attachment 281050 [details] [review]
giscanner: Add modules for preprocessing and building introspection programs using distutils (take iii)

Hi,

This re-does the patch to add the modules to let giscanner use distutils when preprocessing the sources and compiling and linking the dumper program, with LRN's suggestions on fixing builds on MinGW/MSYS incorporated (albeit in a slightly different way).

> giscanner-Add-Modules-Using-distutils

LRN, can you do me a favor: can you revert the patch I mentioned in the line above and apply this patch and see whether the dumper program will run for you?  I think it might help by forcing self.compiler.dll_libraries for the Mingw32CCompiler to [] or ['msvcrt'], as the dumper program shouldn't be linking against the Python DLL (which is depending on msvcr90.dll).  Please let me know whether this patch works better for you.

With blessings, thank you!

Now i get this:

Traceback (most recent call last):

+ Trace 233841

Makefile:3395: recipe for target 'GIRepository-2.0.gir' failed

Won't be able to debug it this morning, maybe later today...

OK, looks like unixcompiler.py does this:
> compiler = os.path.basename(sysconfig.get_config_var("CC"))
and sysconfig.get_config_var("CC") returns None, because _init_nt() does not set CC. Values are stored in _config_vars, which is internal to sysconfig.py. So far i haven't found a way to override it from outside.

Created attachment 281068 [details] [review]
Give CC to sysconfig

Here's a crude fix for this problem. You can probably do this in some other place, earlier, while processing --compiler option, maybe.

Hello LRN,

Thanks for letting me know.  I think the root issue to what you are getting is because I am trying to do self.compiler.add_runtime_library_dir() in ccompiler.py (in get_internal_link_flags()) for all compilers besides MSVC (it is necessary on Linux builds, at least, AFAICT).

As I am not sure whether the DLLs built by MinGW/MSYS land in the current directory or in a .libs sub directory, or both, can you do me a favor: can you try adding a if os.name != 'nt' check for each of the 3 calls to self.compiler.add_runtime_library_dir(), so that we don't do add_runtime_library_dir() on Windows at all and see how the scripts work for you?

I will try to come up with a new patch for the new modules based on your results here.

Thanks for the time, and help, with blessings!

Removing self.compiler.add_runtime_library_dir() calls (same as adding an os.name != 'nt' check) worked just as well as attachment 281068 [details] [review] did.

Created attachment 281291 [details] [review]
giscanner: Add modules for preprocessing and building introspection programs using distutils (take iv)

Hi,

This adds the ccompiler.py and msvccompiler.py modules to giscanner, which attempts to resolve issues that LRN reported in regards to self.compiler.add_runtime_library_dir(), in comment 40.  This has been checked with 'make distcheck' on my Fedora 20 system as well.

LRN, can you apply revert the previous patch on this (attachment 281050 [details] [review]), and see whether this works for you?  Please let me know.

With blessings, thank you!

attachment 281291 [details] [review] works.

What about the new --compiler option? Shouldn't it somehow be incorporated into Makefile.introspection?

Hello LRN,

> attachment 281291 [details] [review] works.

Nice to hear that!

> What about the new --compiler option? Shouldn't it somehow be incorporated into
> Makefile.introspection?

Oh... by the way, did you need to use that option when you built with MSYS, or you cross-compiled?  As I am not that good in autotools, I might be able to do the change to Makefile.introspection, but I won't be able to test it...

Thanks for the info, with blessings!

(...for records, the --compiler option is intended for Windows builds only, due to the case that official Python is built with MSVC, and we might be using MinGW to build g-i, so we need to tell what compiler giscanner uses if it could not determine the correct compiler to use for the build from the environment-whether it is running in an MSYS shell).

>> What about the new --compiler option? Shouldn't it somehow be incorporated into
>> Makefile.introspection?
> 
> Oh... by the way, did you need to use that option when you built with MSYS, or
> you cross-compiled?
No, i didn't need it. Still, configure script is THE thing that knows *precisely* which compiler is to be used. By the time Makefile.introspection is included, this information is available, and i would recommend it to be given to GI.

>  As I am not that good in autotools, I might be able to do
> the change to Makefile.introspection, but I won't be able to test it...
Allright, i'll take a stab at it. Is there a way to disrupt compiler autodetection? That is, make sure it doesn't find the compiler on its own, verify that GI fails to build, and then add the --compiler option to Makefile.introspection and verify that GI builds again.

Hello LRN,

(In reply to comment #49)
> Allright, i'll take a stab at it. Is there a way to disrupt compiler
> autodetection? That is, make sure it doesn't find the compiler on its own,
> verify that GI fails to build, and then add the --compiler option to
> Makefile.introspection and verify that GI builds again.

From what I can tell from the distutils scripts, on *NIX it basically does what is currently done in GI master in regards to compiler detection, that is, querying the CC envvar.  What I guess could be done is, let distutils go its own way on *NIX, as is (since 'make distcheck' does currently pass with these patches), and always pass in the --compiler option when we are building with MinGW, whether in MSYS or possibly, if this is supported already, when we cross-compile for Windows from *NIX.

My take on this-please correct me if I am wrong.

With blessings, thank you!

I've looked at the code more closely, and realized that --compiler does not take the name of the compiler executable (as i thought), but takes a string that identifies compiler class (msvc, mingw32, etc). In this case adding --compiler to Makefile.introspection is neither straightforward, nor useful.

Created attachment 281464 [details] [review]
giscanner/dumper.py: Use the new ccompiler module (v4)

Hi,

This replaces the previous patch on dumper.py on using the new ccompiler.py and msvccompiler.py classes, as it improves on ignoring the failures of mt.exe on Visual Studio 2010 and later builds, as they don't generate a .exe.manifest file when compiling the dumper program.

This has been checked by 'make distcheck' as well.

p.s. The patches can be applied in any order, but all are needed for the build to work.  My application order is, for references:

giscanner: Add modules for preprocessing and building introspection programs using distutils...

Updates to giscanner/scannerlexer.l to support using the MSVC preprocessor

giscanner/dumper.py: Use the new ccompiler module...

shlibs.py: Windows: Use CCompiler module to deduce DLL from library

giscanner/scannermain.py: Add a --compiler option...

sourcescanner: Use distutils for preprocessing the sources

---
With blessings, thank you!

Review of attachment 276057 [details] [review]:

Looks mostly ok, some formatting problems and one question.

::: giscanner/scannerlexer.l
@@ +100,3 @@
 
+"# "[0-9]+" ".*"\n"			{ process_linemarks(scanner, FALSE); }
+"#line "[0-9]+" ".*"\n"		{ process_linemarks(scanner, TRUE); }

Tabulation problem, are you using tab=8 ?

@@ +175,3 @@
+"typedef char __static_assert_t".*"\n"	{ /* Ignore */ }
+"__cdecl"				{ /* Ignore */ }
+"__declspec(dllimport)"		{ /* Ignore */ }

tabs

@@ +177,3 @@
+"__declspec(dllimport)"		{ /* Ignore */ }
+"__declspec (dllimport)"		{ /* Ignore */ }
+"__declspec(dllexport)"		{ /* Ignore */ }

tabs

@@ +182,3 @@
+"__declspec(deprecated(".*"))"		{ /* Ignore */ }
+"__declspec(noalias)"			{ /* Ignore */ }
+"__declspec(dllexport)"		{ /* Ignore */ }

Do you wan't to ignore all declspec's + whitespace combinations? If so these could probably all be combined into something like:
"__declspec("[a-z\t ]+")"	{ /* Ignore */ }

@@ +185,3 @@
+"__declspec("[\t\f\v\r ]+"deprecated".*+"\n"	{ /* Ignore */ }
+"__stdcall"				{ /* ignore */ }
+"__declspec (dllexport)"		{ /* Ignore */ }

tabs

Something that will help getting these reviewed and committed would be to decompose the patches more. For instance the "shlibs.py: Windows: Use CCompiler module to deduce DLL from library" patch should be a code movement patch which only moves the code into a function or method inside of ccompiler.py taking only the required args to make it work "as is". Then a follow up patch which moves code into the initializer of CCompiler and adds usage of things like self.check_is_msvc(). After that a bugfix patch which fixes the current problems you are trying to solve in the related code. I know it is tedious, but it helps reviewing and bisecting regressions.

We also need to think about unit-testing because it will change design a little bit. For instance, it will probably be important for CCompiler to accept an environment dictionary and os name:
    compiler = CCompiler(environ=os.environ, osname=os.name)

This way we can test things as follows:

class TestWindowsCompiler(unittest.TestCase):
    def test_compiler_creation(self):
        compiler = CCompiler(osname='nt', environ={'MSYSTEM': 'MINGW32'})
        self.assertTrue(isinstance(compiler.compiler, MinGW32Compiler)

        compiler = CCompiler(osname='nt', compiler_name='msvc')
        self.assertTrue(isinstance(compiler.compiler, MSVCCompiler)

        with self.assertRaises(SystemExit):
            CCompiler(osname='nt', compiler_name='bad_compiler_name')


That's just one example of very simple things we can do to test the correctness and expectations from any environment.

Created attachment 282244 [details] [review]
Updates to giscanner/scannerlexer.l to support using the MSVC preprocessor [v2]

Hello Simon,

(In reply to comment #53)
> Review of attachment 276057 [details] [review]:
> 
> Looks mostly ok, some formatting problems and one question.

Hmm, my tabs are 4 spaces, I could do a guess on the tabs from the other existing lines notepad++ and looking at GIT GUI when I do the commit on my local g-i repo... Is this patch better in terms of formatting?

> Do you wan't to ignore all declspec's + whitespace combinations? If so these
> could probably all be combined into something like:

Tried to squash the __declspec stuff a bit here, but still need to account for things like __declspec(deprecated(...)) and __declspec (...), not that much of an expert in lexical stuff (tried "__declspec("[a-z()\t ]+")"    { /* Ignore */ }, but seemed not to do the job), so hope this is better for you.

I will try to get the patches sorted in the way that you mentioned in comment 54...

Thanks though for your time, with blessings.

Created attachment 282253 [details] [review]
Add new CCompiler Module, and move portions of dumper.py and shlibs.py there

Hi,

This adds a ccompiler.py module, so that it can be expanded and used later to do preprocessing, compiling and linking work using distutils.  This first moves items from dumper.py (getting the internal and external link args) and shlibs.py (resolve Windows libraries to DLLs) there.

Please see the commit comments for more details.

With blessings, thank you!

Created attachment 282307 [details] [review]
giscanner/sourcescanner.py: Don't use stdin/stdout as the input/output of the preprocessor

Hi,

This does the update to giscanner/sourcescanner.py so that it would not use stdin and stdout as the input/output of the preprocessor, as when we later move to distutils, distutils does not accept stdin/stdout as its input/output when we call its preprocess methods.

This has an added bonus that we will now be able to allow MSVC builds to have introspection files built without using MinGW/GCC as a helper, as using stdin as input to the preprocessor is not allowed under MSVC.

With blessings, thank you!

Created attachment 282308 [details] [review]
MSVC Intropsection Builds: Drop GCC requirement

Hi,

This updates the NMake Makefiles for builds of introspection files to not require GCC, as we are no longer using stdin as the input to the preprocessor, which was why GCC was required as a helper for MSVC introspection builds.

With blessings, thank you!

Created attachment 282309 [details] [review]
Remove Mentions of the GCC requirement in MSVC README.txt's

Hi,

This is in a separate commit as those README.txt files need to be in DOS/Windows format.

Created attachment 282311 [details] [review]
giscanner/ccompiler.py: Initiate Compiler Instance Using Distutils

Hi,

This updates the __init__ constructor method so that we can use ccompiler.py to detect the compiler appropriate with the platform, with some user-supplied options and envvar as needed, so this is the beginning of where we progressively remove the check for the CC envvar for cl (or cl.exe) from the giscanner scripts (as done in ccompiler.py and dumper.py in this patch).

Also, this updates get_external_link_flags() and get_internal_link_flags() so that we stop copying the library (.lib) to introspect to the <namespace>-<namespace_version>.lib convention, as we have been passing in the basename of the .lib file on Windows for quite a while already, meaning that we could do the linking from the passed in .lib directly.

I will get the other patches needed in the next few days-so far these patches do pass 'make distcheck'.

With blessings, thank you!

Created attachment 282428 [details] [review]
giscanner/sourcescanner.py: Use distutils.ccompiler for preprocessing in _parse()

Hi,

This updates ccompiler.py to add a preprocess() method, which will in turn call distutil.ccompiler's preprocess() method, so that sourcescanner._parse() can use it to preprocess the files that were sent to it, so that this process becomes more portable.

As distutil's MSVCCompiler classes do not provide a preprocess() implementation, we provide our own implementation by subclassing the distutils.MSVC9Compiler class.

With blessings, thank you!

p.s.: I am marking the "Add a --compiler option" patch as obsolete as it might not be that useful at this point, unless there is demand to pass in that option to build the introspection files via MinGW/GCC from cmd.exe rather than via MSYS (which should be supported out of the box at the end of this patch series).

Created attachment 282430 [details] [review]
dumper.py: Use distutils to build dumper program

Hi,

This patch updates ccompiler.py by adding compiler() and link(), which is used by dumper.py to pass to distutils.ccompiler's compile() and link() methods so that the building of the dumper program is carried out by distutils.ccompiler.  The compiling and linking patch are done together as they involve items that are closely tied to each other.  Please see the patch's comments for more details about this patch.

I will try to come up with a unit-test script for ccompiler, as suggested by Simon, in the next day or two or so.

With blessings, thank you!

Created attachment 282493 [details] [review]
giscanner/sourcescanner.py: Use distutils.ccompiler for preprocessing in _parse()

Hi,

The original patch for this had some blank lines that were off, so reposting...

Created attachment 282495 [details] [review]
dumper.py: Use distutils to build dumper program

Hi,

The original patch for this had some lines that were off, so reposting...

Created attachment 282502 [details] [review]
giscanner/ccompiler.py: Query MSVC Paths from os.environ

Hi,

As we might be passing in env vars (such as to ask for Mingw32CCompiler instance) to ccompiler.py for the tests (though most probably not in practice), query for the MSVC paths from os.environ all the time when needed, so that:

-The MSSdk env var may be set correctly on non-MSVC 2008 builds
-We don't have to worry that much about passing in bad env vars for the MSVC paths.

This would probably end up being squashed with attachment 282311 [details] [review], though.

With blessings, thank you!

Review of attachment 282244 [details] [review]:

::: giscanner/scannerlexer.l
@@ +177,3 @@
+"__declspec("[a-z\t ]+")"		{ /* Ignore */ }
+"__declspec(deprecated(".*"))"		{ /* Ignore */ }
+"__declspec"[\t ]+"("[a-z\t ]+")"	{ /* Ignore */ }

Could this be changed to:
"__declspec"[\t ]*"("[a-z\t ]+")"

and then: "__declspec("[a-z\t ]+")"
above this be removed?

@@ +179,3 @@
+"__declspec"[\t ]+"("[a-z\t ]+")"	{ /* Ignore */ }
+"__stdcall" 			        { /* ignore */ }
+"__w64"				{ /* ignore */ }

Need an extra tab here.

Review of attachment 282253 [details] [review]:

LGTM. Thanks for breaking things up.

Created attachment 282534 [details] [review]
Fix MSVC Compiler Instance Creation on UNIX

Hi,

This fixes the distutils MSVCCompiler instance creation so that it would also work on UNIX, so that the test on ccompiler.py will run.  This also makes the query of MSVC paths from the environment more carefully, so that it will work even when a custom envvironment is passed into CCompiler.__init__(), and also on *NIX.

With blessings, thank you!

Created attachment 282536 [details] [review]
Add a unit test script for ccompiler.py (distutils compiler instance creation)

Hi,

This is the unit test script that I came up with, so that the ccompiler.py can be tested for compiler instance creation via distutils.

With blessings, thank you!

Review of attachment 282253 [details] [review]:

Hello Simon,

Thanks for the reviews, the patch was pushed as c9308592.

I will look at scannerlexer.l in a bit.

With blessings.

Created attachment 282549 [details] [review]
Updates to giscanner/scannerlexer.l to support using the MSVC preprocessor [v3]

Hi,

Updated patch to scannerlexer.l as suggested by Simon.

With blessings, thank you!

Review of attachment 282307 [details] [review]:

Nice cleanup, just a few comments and questions.

::: giscanner/sourcescanner.py
@@ +284,3 @@
+
+        tmp_fd_cpp, tmp_name_cpp = tempfile.mkstemp(prefix='g-ir-cpp-', suffix='.c')
+        fp_cpp = os.fdopen(tmp_fd_cpp, 'w')

Since the implicit default is 'wt', will that write out CR/LF combos on windows and if so do we want that?

@@ +287,3 @@
+        self._write_preprocess_src(fp_cpp, defines, undefs, filenames)
+
+        tmpfile_basename = tmp_name_cpp[tmp_name_cpp.rfind(os.sep) + 1:

The other possibility here is:

tmpfile_basename = os.path.basename(os.path.splitext(tmp_name_cpp)[0])

@@ +310,3 @@
+            cpp_args += ['-D_CRT_SECURE_NO_WARNINGS']
+            cpp_args += ['-D_CRT_NONSTDC_NO_WARNINGS']
+        if 'cl.exe' in cpp_args or 'cl' in cpp_args:

Probably better practice to use a single tuple constant:

cpp_args += ('-P',
             '-D_USE_DECLSPECS_FOR_SAL',
             ...,
             )

Not that it necessarily matters here, but it is more optimal:

>>> import dis
>>> dis.dis(lambda: [] + ('a', 'b'))
  1           0 BUILD_LIST               0 
              3 LOAD_CONST               3 (('a', 'b')) 
              6 BINARY_ADD           
              7 RETURN_VALUE         

# vs.

>>> dis.dis(lambda: [] + ['a'] + ['b'])
  1           0 BUILD_LIST               0 
              3 LOAD_CONST               1 ('a') 
              6 BUILD_LIST               1 
              9 BINARY_ADD           
             10 LOAD_CONST               2 ('b') 
             13 BUILD_LIST               1 
             16 BINARY_ADD           
             17 RETURN_VALUE

@@ +320,1 @@
+        proc = subprocess.Popen(cpp_args)

I think we should probably call fp_cpp.close() before calling this?

Review of attachment 282308 [details] [review]:

I think this is probably fine. Although it depends on the previous patch...

As a side note, if I want to do an msvc build is this the correct document to follow?
https://wiki.gnome.org/Projects/GTK%2B/Win32/MSVCCompilationOfGTKStack

Review of attachment 282549 [details] [review]:

Sure.

Review of attachment 282309 [details] [review]:

This seems fine, although the patch wasn't applying for me.

Created attachment 283443 [details] [review]
giscanner/sourcescanner.py: Don't use stdin/stdout as the input/output of the preprocessor (take ii)

Hello Simon,

(In reply to comment #72)
> Nice cleanup, just a few comments and questions.
> 

:)
 
> Since the implicit default is 'wt', will that write out CR/LF combos on windows
> and if so do we want that?

I checked it when running from cmd.exe, it's still UNIX line endings...

> The other possibility here is:
> 
> tmpfile_basename = os.path.basename(os.path.splitext(tmp_name_cpp)[0])

Got it here

> @@ +310,3 @@
> Probably better practice to use a single tuple constant:
> 
> cpp_args += ('-P',
>              '-D_USE_DECLSPECS_FOR_SAL',
>              ...,
>              )

Got it here

> @@ +320,1 @@
> +        proc = subprocess.Popen(cpp_args)
> 
> I think we should probably call fp_cpp.close() before calling this?

My bad, I had bad code placement, so shifted the close() call up to write after the _write_preprocess_src() call...

> As a side note, if I want to do an msvc build is this the correct document to
> follow?
> https://wiki.gnome.org/Projects/GTK%2B/Win32/MSVCCompilationOfGTKStack

Yes, that's right.  I try to keep it up-to-date.

About attachment 282309 [details] [review], it needs to be applied with --ignore-white-space, as they are in CR/LF format, and it is intentional.

Thanks for the reviews.  With blessings

Review of attachment 282549 [details] [review]:

Hello Simon,

Thanks for the reviews, the patch was pushed as 29838038.

With blessings.

Created attachment 284175 [details] [review]
giscanner/ccompiler.py: Initiate Compiler Instance Using Distutils (take ii)

Hi,

I thought it might be a good idea that the patch to ccompiler.py and dumper.py for using distutils to instantiate the compiler instance incorporate the fixes (as in patch 282534), and move dumper.py to use ccompiler.py's check_is_msvc() function instead of using things like "if 'cl.exe' in args or 'cl' in args:", since we need to update dumper.py for this change in ccompiler.py already.  So here's the new patch for instantiating the compiler instance in ccompiler.py's __init__() with distutils.

With blessings, thank you!

Created attachment 284188 [details] [review]
giscanner/sourcescanner.py: Use distutils.ccompiler for preprocessing in _parse() (take ii)

Hi,

Like the newer patch to initiate the compiler instance with distutils (attachment 284175 [details] [review]), this patch for preprocessing the source/headers files for introspection also incorporates the fixes in attachment 282534 [details] [review] for msvccompiler.py, so that if we want to create a MSVCCompiler instance on UNIX during testing it will work.

With blessings, thank you!

Created attachment 284368 [details] [review]
giscanner/ccompiler.py: Initiate Compiler Instance Using Distutils (take iii)

Hi,

In bug 734163, there was the need to check for MINGW64 if we are going to use MinGW32 to build the introspection files, which means this is what we also want to check for when we initiate the compiler instance want for MinGW builds, which could be done under MSYS or MSYS2.  This is the slightly updated patch for this, which is a prerequisite for attachment 284188 [details] [review]...

Created attachment 284369 [details] [review]
dumper.py: Use distutils to build dumper program (take ii)

Hi,

This updates the patches to dumper.py (and ccompiler.py) due to the changes brought about by attachment 284268 [details] [review], so that the patch is cleaned up a bit.

With blessings, thank you!

Hello Simon,

Is there any chance that I could have attachment 283443 [details] [review] reviewed (for not using stdin/stdout for running the preprocessor), so that the MinGW/GCC preprocessor requirement can be dropped for building introspection files with MSVC builds, or is there something else that I need to provide for that patch, as we are drawing close to a stable release (if moving the giscanner scripts to distutils is too late for this release cycle)?

Thanks for your time, with blessings!

Created attachment 285218 [details] [review]
giscanner/ccompiler.py: Initiate Compiler Instance Using Distutils (take iv)

Hi,

This patch is updated due to nacho's very recent changes to ccompiler.py, so that the patch will apply again.

With blessings, thank you!

Hi,

Any news or feedback about this patch series, or did I miss anything in the earlier comments?  Haven't heard anything about this in quite a while (about 2.5 months)... I certainly hope that the time and effort invested in this did not go in vain :|.

With blessings, thank you!

Yeah, sorry for not getting back to you about this... it's been really busy lately. I can probably chip away at a review this month and I'll definitely have a lot more time with holidays coming up.

Hi,

I'm adding Nacho to the CC list, as this bug is sitting here for quite a bit pending for further review (after getting his consent for this).  Would definitely like to improve the G-I situation on Windows!

With blessings, thank you!

Dunno, I'd say let's wait for Simon's review since he already has all the context about this. I'd ask LRN too about this.

What order do these patches apply ?

Hello Simon,

Here it goes (I hope I remembered it right :), from my git log, that is :) ):

Attachment 283443 [details] (giscanner/sourcescanner.py: Don't use stdin/stdout as the input/output of the preprocessor (take ii)

Attachment 282308 [details] (MSVC Intropsection Builds: Drop GCC requirement)

Attachment 282309 [details] (Remove Mentions of the GCC requirement in MSVC README.txt's)--note, this patch is in DOS line endings, so one must ignore whitespaces when applying

Attachment 285218 [details] (giscanner/ccompiler.py: Initiate Compiler Instance Using Distutils (take iv))

Attachment 284188 [details] (giscanner/sourcescanner.py: Use distutils.ccompiler for preprocessing in _parse() (take ii))

Attachment 282536 [details] (Add a unit test script for ccompiler.py (distutils compiler instance creation))

Attachment 284369 [details] (dumper.py: Use distutils to build dumper program (take ii))

With blessings, thank you, and Happy Holidays!

[Mass-moving gobject-introspection tickets to its own Bugzilla product - see bug 708029. Mass-filter your bugmail for this message: introspection20150207 ]

Created attachment 298958 [details] [review]
giscanner/sourcescanner.py: Don't use stdin/stdout as the input/output of the preprocessor (take iii)

Hi,

Due to commit d2dce55 (scanner: don't pass certain debug level flags to cpp), we need to update the patch to switch sourcescanner.py to use tempfiles instead of stdint/stdout for the preprocessor.

With blessings, thank you!

Created attachment 298959 [details] [review]
giscanner/sourcescanner.py: Use distutils.ccompiler for preprocessing in _parse() (take iii)

Hi,

Due to commit d2dce55, update the patch to use distutils for preprocessing the headers and sources for introspection to take into account the extra debug flags so that we won't try to add them to processor flags.

The other patches in this bug will still work in their current state, and each patch was checked with 'make distcheck'.

Hopefully this bug can be done for ASAP, as the lengthy time involved in waiting for the reviews meant that patches had to get updated as they bit-rot through the course of time :|

With blessings, thank you!

Hi,

For references, the order for the patches are as follows:

Attachment 298958 [details] (giscanner/sourcescanner.py: Don't use stdin/stdout as the input/output of the preprocessor (take iii)

Attachment 282308 [details] (MSVC Intropsection Builds: Drop GCC requirement)

Attachment 282309 [details] (Remove Mentions of the GCC requirement in MSVC README.txt's)--note, this patch is in DOS line endings, so one must ignore whitespaces when applying

Attachment 285218 [details] (giscanner/ccompiler.py: Initiate Compiler Instance Using Distutils (take iv))

Attachment 298959 [details] (giscanner/sourcescanner.py: Use distutils.ccompiler for preprocessing in _parse() (take iii))


Attachment 284369 [details] (dumper.py: Use distutils to build dumper program (take ii))

Attachment 282536 [details] (Add a unit test script for ccompiler.py (distutils compiler instance creation))

Please note that the at least the last 2 (if not the last 3) patches could be committed in any order.

With blessings, thank you!

Created attachment 307887 [details] [review]
giscanner/ccompiler.py: Partially Revert c9cfa2b

Hi,

The commit c9cfa2b has to be reverted partially for this patch series to work as it removed the import of distutils items (which is actually one of the major areas of this patch series), as the items for this bug is only part-way into master.

This is one of the unfortunate things that happen along the way :|, so here comes the remedy for it...

Fan, this bug has gone insane. Could you push a wip branch with all the patches applied in order and properly working so it is easier to understand what to review and in which order.

Created attachment 307891 [details] [review]
giscanner/sourcescanner.py: Use distutils.ccompiler for preprocessing in _parse() (take iv)

Hi,

This patch is slightly updated so that it will conform to the PEP-8 (1.6.2) checks that was introduced in 2896dc4. ("if not xxx in" -> "if xxx not in")...

Hi Nacho, agreed.

Did that at wip/fanc999/dumper-distutils-didn't see your message until I did comment 96, sorry.

With blessings.

Review of attachment 284369 [details] [review]:

Looks like a good cleanup.

Review of attachment 285218 [details] [review]:

Not going to triple-check all of the hairy logic here, but from a quick pass, it looks like it's doing correct things. Otherwise, going to trust you got it right.

Hi Jasper,

Thanks for the reviews!

As the accepted patches all have prerequisite patches (namely, attachment 298958 [details] [review] and attachment 307887 [details] [review] are needed before attachment 285218 [details] [review], I can squash attachment 307887 [details] [review] into 285218, if that is better; and attachment 307891 [details] [review] and attachment 285218 [details] [review] are needed before attachment 284369 [details] [review]), I just wanted to find out whether these prerequisite patches are ok to go in.

Thanks for the time!

With blessings!

Review of attachment 307887 [details] [review]:

Looks OK.

Review of attachment 298958 [details] [review]:

Also looks good.

Review of attachment 307891 [details] [review]:

Looks good.

Hi Jasper (and Nacho),

Many, many thanks for the help and reviews, so this bug is finally drawing to and end.  The patches were pushed as the following:

attachment 298958 [details] [review] (giscanner/sourcescanner.py: Use Tempfiles For Parsing): 639841e

attachment 282308 [details] [review] (MSVC Intropsection Builds: Drop GCC requirement): 8aabc21

attachment 282309 [details] [review] (MSVC Builds: Update README.txt's):af054fe

attachments 307887 and 285218 (giscanner/ccompiler.py: Partially Revert c9cfa2b, giscanner/ccompiler.py: Initiate Distutils Compiler Instance): squashed together as 0638f9f

attachment 307891 [details] [review] (sourcescanner.py: Use Distutils for Preprocessing): ad8fc51

attachment 284369 [details] [review] (dumper.py: Use Distutils to Build Dumper Program): 7ce646d

The only patch left in this bug not committed is the test script for this transition to distutils (i.e. attachment 282536 [details] [review]).

Many thanks once again.

With blessings!

It seems this patchset broke the build of libgnome-volume-control, which in turn broke the build of gnome-control-center, gnome-settings-daemon, and gnome-shell.

Build log available at:

http://build.gnome.org/continuous/buildmaster/builds/2015/07/30/16/build/log-gnome-shell.txt

I will have to tag gobject-introspection in gnome-continuous until the issue is resolved.

Hi Emmanuele,

(In reply to Emmanuele Bassi (:ebassi) from comment #105)
> It seems this patchset broke the build of libgnome-volume-control, which in
> turn broke the build of gnome-control-center, gnome-settings-daemon, and
> gnome-shell.

I took some time to try to build the latest gnome-shell on my Fedora 22, and it turns out that reverting attachment 284369 [details] [review] (dumper.py: Use Distutils to Build Dumper Program) would remedy the problem (while making sure that 'make distcheck' will still pass for g-i).  So I reverted it just now.

I will see what I can try to do about this to make it work tomorrow, but since I am definitely not a *NIX expert, I would most probably need some help.  Sorry for the inconvenience.

With blessings.

(In reply to Fan, Chun-wei from comment #107)
> Hi Emmanuele,
> 
> (In reply to Emmanuele Bassi (:ebassi) from comment #105)
> > It seems this patchset broke the build of libgnome-volume-control, which in
> > turn broke the build of gnome-control-center, gnome-settings-daemon, and
> > gnome-shell.
> 
> I took some time to try to build the latest gnome-shell on my Fedora 22, and
> it turns out that reverting attachment 284369 [details] [review] [review] (dumper.py:
> Use Distutils to Build Dumper Program) would remedy the problem (while
> making sure that 'make distcheck' will still pass for g-i).  So I reverted
> it just now.

Thanks, very much appreciated.

> I will see what I can try to do about this to make it work tomorrow, but
> since I am definitely not a *NIX expert, I would most probably need some
> help.  Sorry for the inconvenience.

No worries.

It seems that the get_internal_link_flags() is not working properly in identifying the dependencies encoded in the .la file for libtooled, internal libraries.

Attachment 284369 [details] also breaks gobject-introspection build in jhbuild on FreeBSD because distutils doesn't use LDFLAGS when linking executables.

Created attachment 308527 [details] [review]
giscanner: Use Distutils to Compile and Linker Dumper Program (take iii)

Hi Emmanuele,

Thanks for the notes, which I hope allowed me to do the needed things.

This is the updated version of attachment 284369 [details] [review], where we actually try to parse the libtool library during an "internal" link, in addition to what is done.  The rationale behind this is that we need to acquire the correct libraries and libpaths from the libtool (.la) file during "internal" linking so that the needed libraries can be found.  I have checked it again doing a gnome-shell build and it seems that it is okay.

I also checked the LDFLAGS part on my Fedora 22 system (I don't have FreeBSD, so can't say much about it), the LDFLAGS part are picked up by distutils when doing customize_compiler(), and I don't see anything different in the FreeBSD case.

Please also allow to say hi to a fellow person involved in GNOME developement from my country (Ting-wei Lan), in my language :)

----
挺瑋大：

多謝您的回應，也很高興在這裡見到台灣的同好，也在此跟您問候。
----
With blessings, thank you!

...in the BSD Case, meaning judging from the distutils scripts from Python...

Attachment 308527 [details] still doesn't work on FreeBSD.                                 
                                                                                 
https://hg.python.org/cpython/file/309cbbc32491/Lib/distutils/sysconfig.py#l196  
distutils uses different commands to link shared libraries and executables. linker_so contains LDFLAGS, but linker_exe doesn't. When g-ir-scanner links the executable, LDFLAGS aren't included because linker_exe is used. This causes the error when making GLib-2.0.gir:

/usr/bin/ld: cannot find -lintl

If I workaround the problem by running 'gmake CC="$CC $LDFLAGS"', another message is showed because clang doesn't know -export-dynamic. It converts '-export-dynamic' to '-e xport-dynamic' and sends it to the linker:

/usr/bin/ld: warning: cannot find entry symbol xport-dynamic; defaulting to      
00000000004028d0

----
我也很高興在這裡遇到同樣來自臺灣的人。
----

Created attachment 308932 [details] [review]
giscanner: Use Distutils to Compile and Linker Dumper Program (take iv)

Hi Ting-wei,

Can you try this patch to see whether it works for you?

With blessings, thank you!

---
還有，你那邊颱風侵襲時期還好嗎?這次蠻大的，很久沒這樣了...
---

These messages are still showed:

/usr/bin/ld: warning: cannot find entry symbol xport-dynamic; defaulting to 00000000004028d0
/usr/bin/ld: warning: cannot find entry symbol xport-dynamic; defaulting to 0000000000401b40
/usr/bin/ld: warning: cannot find entry symbol xport-dynamic; defaulting to 0000000000401910
/usr/bin/ld: warning: cannot find entry symbol xport-dynamic; defaulting to 0000000000407a00

Can we use -Wl,--export-dynamic instead of -export-dynamic?

---
這次風好大，從外面吹了一些東西進來 ......
---

Build failure found in at-spi2-core:

  GISCAN   Atspi-2.0.gir
/usr/bin/ld: cannot find -ldbind
cc: error: linker command failed with exit code 1 (use -v to see invocation)

Hi,

Since this bug has already fulfilled its original intention, i.e. supporting .gir generation on Visual C++ without needing a Windows GCC as a helper, I am closing this bug as "resolved-fixed", and as it is clearly seen, this bug became way bigger than originally intended.

So, I am redirecting further work and progress in bug 753428, where we can finish off the efforts here to make the .gir dumper binary build with distutils.

Thank you all of the feedback and suggestions along the way.

With blessings, thank you!
---

Hi Ting-wei,

Hmm, I see... I would most probably need some suggestions/help from you on getting things work under FreeBSD and/or jhbuild, as that is not my native territory.  I'll see what I can do though...

多謝，願一切平安