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

avoid mpileup writing to inputdir #3951

Merged
merged 2 commits into from
Sep 21, 2021

Conversation

pavanvidem
Copy link
Member

@pavanvidem pavanvidem commented Sep 13, 2021

FOR CONTRIBUTOR:

  • - I have read the CONTRIBUTING.md document and this tool is appropriate for the tools-iuc repo.
  • - License permits unrestricted use (educational + commercial)
  • - This PR adds a new tool or tool collection
  • - This PR updates an existing tool or tool collection
  • - This PR does something else (explain below)

@bgruening
Copy link
Member

@pavanvidem your medaka commit is still in here. Can you please clean that.

ping @natefoo

ln -s '${reference_source.ref_file}' 'ref.fa' &&
samtools faidx ref.fa &&
#else:
#set ref_fa = str( $reference_source.ref_file.fields.path )
Copy link
Member

Choose a reason for hiding this comment

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

is the str() needed here?

Copy link
Contributor

Choose a reason for hiding this comment

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

also, ref.fa doesn't need quoting, but $reference_source.ref_file.fields.path might.

Copy link
Member Author

Choose a reason for hiding this comment

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

maybe. I just copied it from calmd here

#set ref_fa = str( $reference_source.ref_fasta.fields.path )

@bernt-matthias
Copy link
Contributor

Maybe we should integrate this in #3740 .. and check if other samtools wrappers do the same mistake.

@bernt-matthias
Copy link
Contributor

Ideally the tool should use the @PREPARE_FASTA_IDX@ token? Probably the conditional and maybe in included parameters need to be renamed.

sam_to_bam and calmd could be treated the same way.

@bernt-matthias
Copy link
Contributor

I will merge this into #3740

@wm75
Copy link
Contributor

wm75 commented Sep 15, 2021

@bernt-matthias I had assumed the missing version bump was on purpose here to fix the tool silently?
Merging things into the larger samtools update will leave public servers with an executable tool version that misbehaves.

@bgruening @natefoo am I misunderstanding things?

@bernt-matthias
Copy link
Contributor

@wm75 .. I see. Then lets merge, or @bgruening?

@bernt-matthias
Copy link
Contributor

The changes that I have suggested are now implemented in #3740

I also cherry-picked the commit from the PR which should not hurt .. maybe I need to merge after this one is in.

@bgruening
Copy link
Member

ping @pavanvidem

@pavanvidem
Copy link
Member Author

I also implemented the suggested changes today morning. I'm pushing my changes now, then I will check if there something that I can adapt from #3740

@bernt-matthias
Copy link
Contributor

I also implemented the suggested changes today morning. I'm pushing my changes now, then I will check if there something that I can adapt from #3740

I would suggest to keep it as is, i.e. pavanvidem@b12876b

Otherwise we might need a version bump and loose the possibility for a "silent update".

@pavanvidem
Copy link
Member Author

The changes shouldn't affect any functionality. Can't we do a silent update here?

@bernt-matthias
Copy link
Contributor

The changes shouldn't affect any functionality. Can't we do a silent update here?

Renaming parameters might affect workflows. So I would say: No

@pavanvidem
Copy link
Member Author

Agree, makes sense.

@bernt-matthias
Copy link
Contributor

Good to go @bgruening and @wm75?

Copy link
Contributor

@wm75 wm75 left a comment

Choose a reason for hiding this comment

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

LGTM. Sorry for the back and forth here @pavanvidem

@pavanvidem
Copy link
Member Author

all good @wm75, thank you and @bernt-matthias for the review

@bernt-matthias
Copy link
Contributor

I will merge this if @bgruening does not object..

@bernt-matthias bernt-matthias merged commit a18f79e into galaxyproject:master Sep 21, 2021
@pavanvidem pavanvidem deleted the mpileup_fix branch July 9, 2024 14:47
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.

4 participants