Closed kenza12 closed 2 years ago
I think the real culprit here is the use of filecmp.cmp
. We should not be doing file comparisons because of cross-platform differences in line endings. Instead of forcing the Unix line ending of LF, I think the comparison could be done by reading the files using universal newlines. Here is a snippet from a new test in pySBOL3 that @jakebeal wrote which could be used as an example:
def test_write_windows(self):
# Make sure that Windows doesn't change line endings into CRLF
test_path = os.path.join(SBOL3_LOCATION, 'entity', 'model',
'model_ordered.nt')
with open(test_path, 'r') as infile:
expected = infile.read()
# Rewrite file in NTRIPLES, which should produce exact same thing:
doc = sbol3.Document()
doc.read(test_path)
with tempfile.TemporaryDirectory() as tmpdirname:
test_file = os.path.join(tmpdirname, 'tmp.nt')
doc.write(test_file, sbol3.SORTED_NTRIPLES)
with open(test_file, 'r') as infile:
actual = infile.read()
# Check if lines differ (which on Windows would be due to CRLF instead of just newline)
self.assertEqual(expected, actual)
Here the files are read using universal newlines (the default), and then the contents are compared. I recommend you change the unit test to do this instead of filecmp.cmp
. I do not think you should be forcing the Unix line ending when writing the file. That happens to work, and will break if the test_files/mini_library.nt
were ever changed to include Windows line endings (CRLF). Using universal newlines will be robust to this change. The N-Triples file format allows either LF or CRLF.
Hi @bbartley , thank you for merging. Is it possible to create a new tag including these modifications please ? Thanks in advance.
@kenza12 I have tagged a new release here: https://github.com/SynBioDex/sbol_factory/releases/tag/v1.0a12
FYI it was necessary to modify your original test solution. I believe there was a discrepancy in how your local github repo is configured compared to the Github actions CI environment. The CI environment is configured to automatically convert between Linux and Windows line endings. I suspect your local repo might be configured to conserve Linux line endings.
In any case, the tests should now be completely agnostic about line endings. Hopefully they will now pass on your machine.
Hi @bbartley , Thank your for the tag. The solution that you provided is also working. The published conda package is built and tested on Anaconda Platform (not from my local Github repo). And they are using CircleCI for tests: https://dev.azure.com/conda-forge/feedstock-builds/_build/results?buildId=508118&view=logs&jobId=6f142865-96c3-535c-b7ea-873d86b887bd&j=e35d4f76-8ff2-5536-d795-df91e63eb9f7 They use this following tarball : https://github.com/SynBioDex/sbol_factory/archive/v1.0a12.tar.gz But I also tested it locally (the old version) from my Github repo on windows and the test failed with the same error as on anaconda Platform. This is probably due to Github configuration as you said. Anyway it's working perfectly now ! Thanks again.
Hi @bbartley , I have another little issue with sbol_factory reported by a conda-forge reviewer. He asked me to provide a stable release instead of a pre-release (https://github.com/SynBioDex/sbol_factory/releases/tag/v1.0a12) in order to be published. Do you think it's possible for you to change that ? Thank you.
@kenza12 I have removed the pre-release designation on the a12 release. Will this satisfy your conda reviewer?
@kenza12 I have removed the pre-release designation on the a12 release. Will this satisfy your conda reviewer?
Thanks @bbartley , but he said that the version identifier 1.0a12 parses as alpha in pep440 and need to be changed to issue a tag/version that parses as stable: https://github.com/conda-forge/staged-recipes/pull/18861 Is it possible for you to change that ? Thank you
@kenza12 I have removed the pre-release designation on the a12 release. Will this satisfy your conda reviewer?
Thanks @bbartley , but he said that the version identifier 1.0a12 parses as alpha in pep440 and need to be changed to issue a tag/version that parses as stable: conda-forge/staged-recipes#18861 Is it possible for you to change that ? Thank you
Hi @bbartley , I would like to know if you had the chance to address this issue ? Thank you.
@kenza12 I have tagged and deployed a v1.0 release. Thanks for your patience and persistence.
Thanks @bbartley , the sbol_factory v1.0
is published here : https://anaconda.org/conda-forge/sbol_factory
Hi, I'm trying to make a conda package for sbol_factory but I got some test errors in windows:
You can see the detailed errors here: https://dev.azure.com/conda-forge/feedstock-builds/_build/results?buildId=502209&view=logs&jobId=240f1fee-52bc-5498-a14a-8361bde76ba0&j=240f1fee-52bc-5498-a14a-8361bde76ba0&t=06d62fd2-ab49-5add-9ee5-83b6882d7d2b
The problem is that Unix and Windows have different signatures for line endings (CRLF for windows (equivalent to \r\n) and LF for linux (equivalent to \n)). That's why the 2 tests failed for windows.
In this PR, I fixed this issue where I specified '\n' as a newline instead of '\r\n' to make sure that the tests pass for windows as well.
Can you check that please and see if it suits you ?
On the other hand, I wanted to let you know that pysbol2 is updated and packaged in anaconda : https://anaconda.org/conda-forge/pysbol2/files (v1.4.1) as well as pysbol3 (1.0.1): https://anaconda.org/conda-forge/pysbol3/files.
You can install these 2 packages this way:
For pysbol2:
For pysbol3:
Best wishes,
Kenza