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

let demo_content target in makefile run multiple times #295

Closed

Conversation

phette23
Copy link

@phette23 phette23 commented Oct 4, 2022

To test:

  • run make demo_content two or more times
  • second time throws a python error ( see below)
  • apply patch
  • run make demo_content multiple times
  • no error

I ran into some trouble building the demo site and had to run make demo multiple times, but on iterations after the first I kept seeing this error related to running setup.py in Islandora workbench:

File "/workbench/setup.py", line 7
    author_email="mjordan@sfu", packages=["i7Import", "i8demo_BD", "input_data"], packages=["i7Import", "i8demo_BD", "input_data"],
                                                                                  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
SyntaxError: keyword argument repeated: packages

I tracked the problem back to the final sed command in the demo_content target; it replaces "author_email="mjordan@sfu", with "author_email="mjordan@sfu", packages=[... but it's happy to do that multiple times, leading to the error above. Most other things in the repeat run seemed to work despite some warnings. I just modified the sed so it'll only run the substitution on the line if the line doesn't already have "packages" listed. I also removed sed's "g" flag since I don't think a global substitution makes sense here.

I realize this is an edge case since people shouldn't be running make demo multiple times without cleaning things out in between but it came up for me so I figured I'd submit a PR.

python complains if you list a 'packages' property more than once
this change makes `sed` add the packages list only if it is not
already present
@seth-shaw-asu seth-shaw-asu requested a review from alxp October 5, 2022 17:15
@alxp
Copy link
Contributor

alxp commented Oct 6, 2022

I was able to run "make demo_content" twice in a row with no occurrence of the "syntax error" message.

Un-assigning myself since I'm not an ISLE expert but it seems OK to merge regardless if it is reproducible elsewhere.

@alxp alxp removed their request for review October 6, 2022 02:39
@adam-vessey
Copy link
Contributor

adam-vessey commented Oct 12, 2022

PR is presently in conflict; however, could this potentially be due to the other issue, with different flavours of sed?... though #291 also indicates that it addresses the idempotency of the sed invocation, so this may no longer be an issue?

@phette23
Copy link
Author

I just tested with a fresh isle-dc at HEAD on my laptop and running make demo_content twice in a row didn't throw any errors. The other sed edit in f44ddc1 fixed this because it added $$ to the end of the sed substitution; now the second run won't match the substitution pattern because the comma after the email is no longer the end of the line.

So I'll close this PR, there's no need to merge it. The $$ change is less text, more elegant anyways.

@phette23 phette23 closed this Oct 14, 2022
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.

3 participants