Skip to content

Port to Windows #482

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

Merged
merged 5 commits into from
Aug 12, 2015
Merged

Port to Windows #482

merged 5 commits into from
Aug 12, 2015

Conversation

pjz
Copy link
Contributor

@pjz pjz commented Aug 3, 2015

Appveyor is now running the code under windows... and finding platform-specific bugs.

@pjz pjz force-pushed the fix_appveyor branch 3 times, most recently from 7f3ef83 to 1d2f2ca Compare August 3, 2015 02:24
@pjz pjz force-pushed the fix_appveyor branch 3 times, most recently from 039cd4a to e545217 Compare August 3, 2015 03:01
@pjz
Copy link
Contributor Author

pjz commented Aug 3, 2015

Only 97 failed tests under Windows now. A lot look to be some kind of filesystem-tree breakage.

@pjz
Copy link
Contributor Author

pjz commented Aug 3, 2015

Hey @techtonik want to review this for me? Give me a 👍 and I'll merge it; then we can squish windows bugs.

@@ -120,7 +121,7 @@ def get_wildleaf_fallback():
wild_nonleaf_ns = [ n for n in maybe_wild_nodes if not is_leaf_node(n) ]

# store all the fallback possibilities
remaining = reduce(traverse, nodepath[depth:])
Copy link
Contributor

Choose a reason for hiding this comment

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

Where was traverse coming from? Is it used elsewhere? Should this PR delete it?

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 passed in as a param, and is used correctly ( to join fs paths ) in other places. This spot was actually rejoining URL path parts.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, gotcha.

DESCRIPTION
    Instead of importing this module directly, import os and refer to
    this module as os.path.  The "os.path" name is an alias for this
    module on Posix systems; on other systems (e.g. Mac, Windows),
    os.path provides the same operations in a manner specific to that
    platform, and is an alias to another module (e.g. macpath, ntpath).

    Some of this can actually be useful on non-Posix systems too, e.g.
    for manipulation of the pathname component of URLs.

@chadwhitacre
Copy link
Contributor

Where did pylint sneak in? From the Appveyor template we started with? Let's turn it off.

@chadwhitacre
Copy link
Contributor

... or teach it about our coding style.

@chadwhitacre
Copy link
Contributor

Coverage report looks very broken as well. Sums to 0%.

@chadwhitacre
Copy link
Contributor

Why are the skipped tests skipping? Iirc they are skipping because of an "is on Windows" condition. If so let's review and potentially remove the skip.

@pjz
Copy link
Contributor Author

pjz commented Aug 9, 2015

I think pylint and coverage are because we're running the same target (analyse) here as we do on Travis. Maybe it should be just 'test' instead?

@chadwhitacre
Copy link
Contributor

A lot look to be some kind of filesystem-tree breakage.

Hence AspenWeb/filesystem_tree.py#5?

@chadwhitacre
Copy link
Contributor

I think pylint and coverage are because we're running the same target (analyse) here as we do on Travis.

I am seeing pylint on Travis for Python 2.7 (and it looks like the version of pylint we're using uses set literal syntax not supported under 2.6). Coverage is fine under both 2.6 and 2.7. Out of scope for this PR.

@chadwhitacre
Copy link
Contributor

We should test on Python 2.6 at Appveyor.

@chadwhitacre
Copy link
Contributor

And still, the coverage report on AppVeyor sums to 0%, whereas the coverage report for the same Python version on Travis sums to 62%. @pjz If you don't want to fix that here then we should at least reticket so we remember to check again after we squish all the Windows bugs (which could be causing the discrepancy, I suppose).

@techtonik
Copy link
Contributor

I got back from offline. Let me see what is happening here.

@chadwhitacre
Copy link
Contributor

@techtonik Welcome back! :-)

In my view we are trying to bring AppVeyor at least up to par with what we have at Travis. TODOs:

@chadwhitacre
Copy link
Contributor

I reconfigured our AppVeyor account to represent Gratipay and not whit537, using the @gratipay-gremlin account to link with GitHub.

@chadwhitacre
Copy link
Contributor

Hey @techtonik want to review this for me?

I'm applying Review so this PR shows up on the Radar.

@techtonik
Copy link
Contributor

I'd first improve visibility of the issue by placing badges in #484, but I need API key for that.

@chadwhitacre
Copy link
Contributor

I need API key for that.

Do you? See #484 (comment).

@pjz
Copy link
Contributor Author

pjz commented Aug 12, 2015

So should I turn off pylint and code coverage here?

@chadwhitacre
Copy link
Contributor

@pjz The tasks I see for this PR are at #482 (comment). I don't mind about pylint since it's on Travis and I think we are trying to achieve parity with Travis in order to merge this PR.

@pjz pjz removed the Review label Aug 12, 2015
@pjz
Copy link
Contributor Author

pjz commented Aug 12, 2015

I'm going to go ahead and fix the README thing here too, since it's not worth doing until this is in anyway.

@pjz
Copy link
Contributor Author

pjz commented Aug 12, 2015

What license is aspen under anyway? I don't see it listed anywhere.

@pjz
Copy link
Contributor Author

pjz commented Aug 12, 2015

ah, there it is. MIT.

@pjz pjz force-pushed the fix_appveyor branch 2 times, most recently from dc29727 to ebf718a Compare August 12, 2015 18:03
@pjz
Copy link
Contributor Author

pjz commented Aug 12, 2015

Okay, ready for review again. Also turned README.txt into README.rst so that I could then add an AppVeyor shield to it. Also added the shield to the aspen.io site.

@pjz
Copy link
Contributor Author

pjz commented Aug 12, 2015

Ping @whit537

@pjz pjz mentioned this pull request Aug 12, 2015
@chadwhitacre
Copy link
Contributor

Tests passing on Mac OS.

@chadwhitacre
Copy link
Contributor

Coverage is still borked at 0%. Wanna fix that or reticket, @pjz?

@pjz
Copy link
Contributor Author

pjz commented Aug 12, 2015

reticket, pls.

@chadwhitacre
Copy link
Contributor

It looks like the two skips are still appropriate, yes @pjz?

@pjz
Copy link
Contributor Author

pjz commented Aug 12, 2015

Oh, and I verified the skips are legit: 'Windows file locking makes these fail'

@chadwhitacre
Copy link
Contributor

Reticketed #488.

@chadwhitacre
Copy link
Contributor

Oh, and I verified the skips are legit: 'Windows file locking makes these fail'

Cool. ;-)

chadwhitacre added a commit that referenced this pull request Aug 12, 2015
@chadwhitacre chadwhitacre merged commit b885671 into master Aug 12, 2015
@chadwhitacre chadwhitacre deleted the fix_appveyor branch August 12, 2015 19:30
@chadwhitacre
Copy link
Contributor

Congratulations @pjz @techtonik. Aspen is (re-)ported to Windows, now with continuous integration to ensure we keep it that way! 👯

@techtonik
Copy link
Contributor

👍

Changaco pushed a commit that referenced this pull request Mar 11, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants