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

Bug fix?: rm imports/ro_terms_combined.txt #477

Open
joeflack4 opened this issue Mar 21, 2024 · 4 comments
Open

Bug fix?: rm imports/ro_terms_combined.txt #477

joeflack4 opened this issue Mar 21, 2024 · 4 comments
Assignees
Labels
bug Something isn't working ease:high

Comments

@joeflack4
Copy link
Contributor

joeflack4 commented Mar 21, 2024

Overview

During #471, I noticed that multiple files were still being rm'ed at the end of build-mondo-ingest (original thread). rm imports/ro_terms_combined.txt is one of those files. This seems like a bug to me. I don't remember this being removed before, and I don't know why it should be now.

Possible solution

a. In ODK: Add .PRECIOUS

If this is indeed an issue, the fix should go in ODK because, similar to the other issues in the thread linked above, I imagine that the fix for this is going to be to mark the target as .PRECIOUS. The target is declared in Makefile, which is owned by ODK.

b. If I fix this in mondo-ingest, I think it involves making a copy of the Makefile goal in mondo-ingest.Makefile and then marking that .PRECIOUS.

Additional details

For reference, here's the goal declaration:

$(IMPORTDIR)/%_terms_combined.txt: $(IMPORTSEED) $(IMPORTDIR)/%_terms.txt

Questions

  1. Has this always been happening?
    1.2. If not, why is it happening now?
  2. Why this particular target? (same question applies to other things in thread linked in Overview)
  3. Why this particular target variation? E.g. why does ro_terms_combined get removed, but omo_terms_combined.txt does not? (same question applies to other things in thread linked in Overview)
  4. Should we try to figure out if there is some deeper, strange root cause for this and try to address that, as opposed to doing the .PRECIOUS fix?
  5. Should we forget about this since it is low priority, since it looks like we don't care about this file anyway? I say that, because I see this pattern in the .gitignore: src/ontology/imports/*_terms_combined.txt
@joeflack4 joeflack4 self-assigned this Mar 21, 2024
@joeflack4 joeflack4 added the bug Something isn't working label Mar 21, 2024
@joeflack4
Copy link
Contributor Author

@matentzn @twhetzel Can you guys take a look at my questions and suggested solution?

@joeflack4 joeflack4 mentioned this issue Mar 21, 2024
9 tasks
@matentzn
Copy link
Member

matentzn commented Mar 25, 2024

This is a good question - I would suggest to

  • move this issue over to ODK, and reduce it to the heart of the matter:

SHOULD imports/*_terms_combined.txt be .PRECIOUS?

I think it never was, but somehow I think it should.. I would like to run this by the ODK developers team which meets every four week to discuss those issues.

  • In the meantime, this issue is a low-mid priority, and if you like you can patch it here in Mondo ingest by adding the .PRECIOUS declaration to mondo-ingest.Makefile with a comment linking to this issue here.

@joeflack4
Copy link
Contributor Author

@joeflack4
Copy link
Contributor Author

If I fix this in mondo-ingest, I think it involves making a copy of the Makefile goal in mondo-ingest.Makefile and then marking that .PRECIOUS.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working ease:high
Projects
None yet
Development

No branches or pull requests

2 participants