Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add the builded gtk paths (INCLUDE, LIB, LIBPATH, PATH) to the vs env… #125

Closed
wants to merge 6 commits into from
Closed

Conversation

guruDanny67
Copy link
Contributor

…ironment

Resolve #124.

I have to move up the platform detection & normalization (x86 -> Win32) before checking the vs presence otherwise or the gtk path was missing (attribute error) or badly set (the default is x86 so I end up with a path of c:\gtk-build\gtk\x86.... instead of .. gtk\win32\ ..

Copy link
Contributor

@nacho nacho left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See the comments inline

build.py Outdated

if not os.path.exists(vcvars_bat):
raise Exception("'%s' could not be found. Please check you have Visual Studio installed at '%s' and that it supports the target platform '%s'." % (vcvars_bat, opts.vs_install_path, opts.platform))

# Add to the environment the gtk paths so meson can find everything
self.add_env('INCLUDE', self.gtk_dir + r'\INCLUDE')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know that on win upper case does not matter but I'd rather use lower case since it is what we generate, also for consistency do not align the parameters for different calls

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've got the doubt and I miss both :(

The first version has the environment in lowercase but it feels (to me) a little bit wrong: historically the environment was on windows all uppercase so I upper all. Not a big deal, I fix it at once.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's too late, I need to rest. The path (r'\INCLUDE') should be lower case, not the set variable, right ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah just the path, the environment variable should still be uppercase

build.py Outdated
@@ -1678,12 +1677,23 @@ class Builder(object):
def __init__(self, opts):
self.opts = opts

self.__check_tools(opts)
self.__check_vs(opts)
# Check and normalize the platform
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd rather see this as a different patch

build.py Outdated
@@ -1740,11 +1751,18 @@ def __check_vs(self, opts):
if not os.path.exists(vcvars_bat):
raise Exception("'%s' could not be found. Please check you have Visual Studio installed at '%s' and that it supports the target platform '%s'." % (vcvars_bat, opts.vs_install_path, opts.platform))

# Add to the environment the gtk paths so meson can find everything
self.add_env('INCLUDE', self.gtk_dir + r'\include')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sorry to not point out this before, but can we use os.path.join instead of that +?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, you're completely right.

BTW I finally (once, but I'm confident for the future) been able to do the rebase to squash the commits: I think that my mistake was not using the -f in the final push so git told me that I was not syncronzed with all the refs and then I start to push, pull, fetch, drink & jump, random, to try to fix it, ending up with 10 commits :(

@nacho
Copy link
Contributor

nacho commented Mar 3, 2017

Looks good. Can you squash them?

@guruDanny67
Copy link
Contributor Author

Squashed, I hope (twice, because the first time I forgot a conflict marker).

@nacho
Copy link
Contributor

nacho commented Mar 3, 2017

mmm, there is something wrong, I see 6 commits...

@guruDanny67
Copy link
Contributor Author

Arghhh! These are coming form before I use the squash, so the last three commits are merged but the old one not, sorry.

I opened a new pull request (#127) with the clean commit, sorry.

@nacho
Copy link
Contributor

nacho commented Mar 3, 2017

Closing this one.

@nacho nacho closed this Mar 3, 2017
@guruDanny67 guruDanny67 deleted the gtk-paths branch March 7, 2017 22:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants