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

[SR-6978] SwiftPM ignores vX.X.X style tags if there are X.X.X style tags in a repository #4836

Closed
ankitspd opened this issue Feb 11, 2018 · 17 comments
Labels

Comments

@ankitspd
Copy link
Member

Previous ID SR-6978
Radar rdar://problem/40205112
Original Reporter @aciidb0mb3r
Type Bug
Status Resolved
Resolution Done
Additional Detail from JIRA
Votes 0
Component/s Package Manager
Labels Bug, StarterBug
Assignee None
Priority Medium

md5: 0a54bd5bf1f858b319d8e3b5a8b5f5e3

Issue Description:

We should load both styles and warn in case there is a duplicate.

@belkadan
Copy link

I'm not sure about the warning part. You might need both to support two different systems. But you could warn if they're out of sync, I guess.

@ankitspd
Copy link
Member Author

Yeah, warning if the duplicate points to different SHA sounds good. That might be a little difficult to implement though (because of how things work currently).

@swift-ci
Copy link
Contributor

Comment by Miguel Salinas (JIRA)

Ok, I've started poking around and I think I understand the problem. I wanted some clarification though. In the case that there is a duplicate that points to a different SHA in the git repository, I understand we should warn, but should we default to the X.X.X tag or the vX.X.X tag?

@ankitspd
Copy link
Member Author

I want to say default to the one with the highest count but I am not very sure thats a good idea. Lets default to X.X.X for now.

@swift-ci
Copy link
Contributor

Comment by Miguel Salinas (JIRA)

I implemented my fix on my own fork. This is my first time committing to SPM so please bear with me. It worked in all the situations I tried it in, but I'm not totally confident in my variable naming and overall 'Swiftiness'. Here's a link to my fork https://github.com/Vercantez/swift-package-manager What should I do next? Should I make a Pull Request?

@ankitspd
Copy link
Member Author

Awesome! The next step is to create a PR against the main repository. This guide should help https://help.github.com/articles/creating-a-pull-request/

Feel free to ask any more questions you have in the contribution process.

@swift-ci
Copy link
Contributor

Comment by Miguel Salinas (JIRA)

Thank you! Should I include the issue number in the commit that's in the PR?

@ankitspd
Copy link
Member Author

Yep, that'll be nice. Its not a requirement right now but its good practice in general.

@swift-ci
Copy link
Contributor

Comment by Miguel Salinas (JIRA)

Ok, I made a PR. Is there anything else I should do or do should I just wait for someone to review it? Also, is it ok to add a commit to the PR just cleaning up some whitespace and adding comments?

@ankitspd
Copy link
Member Author

Thats all you need to do! I'll take a look at the PR in the morning (California time) and sorry for the delay 🙁. It is perfect okay to add more commits to the PR.

@swift-ci
Copy link
Contributor

Comment by Miguel Salinas (JIRA)

Thanks!

@ankitspd
Copy link
Member Author

ankitspd commented Mar 5, 2018

Reverted fix in #1520

@swift-ci
Copy link
Contributor

swift-ci commented Mar 5, 2018

Comment by Miguel Salinas (JIRA)

Really sorry about that. I should have made better test cases. I’ll look into fixing it if that’s ok.

@ankitspd
Copy link
Member Author

ankitspd commented Mar 5, 2018

No worries, this is perfectly normal! Feel free to raise another PR.

@swift-ci
Copy link
Contributor

swift-ci commented Mar 6, 2018

Comment by Miguel Salinas (JIRA)

Oh my goodness... I just found out what was causing the bug I introduced. Very dumb mistake; I accidentally removed a condition on a for loop because I copy-pasted from another similar loop I wrote. Essentially, most of the prerelease tags were being considered as the same version. I'll write up a test checking for this sort of thing.

@ankitspd
Copy link
Member Author

@swift-ci create

@ankitspd
Copy link
Member Author

Resolved by #1524

@swift-ci swift-ci transferred this issue from apple/swift-issues Apr 25, 2022
@shahmishal shahmishal transferred this issue from apple/swift May 4, 2022
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants