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

Add support for PR uploads #4041

Merged
merged 1 commit into from
Dec 23, 2017
Merged

Add support for PR uploads #4041

merged 1 commit into from
Dec 23, 2017

Conversation

tresf
Copy link
Member

@tresf tresf commented Dec 6, 2017

Adds support for leveraging upload artifacts for PRs. Targeted at the master branch only.

Justification and planning available in #4035; closes #4035.

Also removes Qt5 from the build system, closing #2611.

@tresf tresf force-pushed the transfer branch 2 times, most recently from 8c3e216 to 06b2516 Compare December 6, 2017 17:31
@tresf tresf changed the title Add support for transfer.sh Add support for file.io.sh Dec 6, 2017
@tresf tresf changed the title Add support for file.io.sh Add support for file.io Dec 6, 2017
echo "Uploading $PACKAGE to file.io..."
response=$(curl -F "file=@$PACKAGE" "https://file.io")
# We need stdout, disable SC2181
# shellcheck disable=SC2181
Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm wondering if there's another way to do it. Can't figure it out now, but will think about it.

Copy link
Member Author

@tresf tresf Dec 6, 2017

Choose a reason for hiding this comment

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

I was thinking something like if [ foo=$(/usr/bin/true) ]; perhaps. Haven't tested it and it's a tad bit uglier.

i.e.

if [ response=$(curl -F "file=@$PACKAGE" "https://file.io") ]; then
# ...

Copy link
Contributor

Choose a reason for hiding this comment

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

Haha, I tried it already, doesn't work. :p (Plus, there's some new warnings from SC because it thinks we want to compare response etc.)

Copy link
Contributor

Choose a reason for hiding this comment

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

OK, actually, it seems to work, but, here's what I got from shellcheck:

 β ~ cat tmp 
#!/bin/bash

if [ response=$(curl -F "file=@$PACKAGE" "https://file.io") ]; then
	echo cc
fi


 λ ~ ./tmp 
Warning: setting file   failed!
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100   145    0     0    0   145      0    145 --:--:-- --:--:-- --:--:--   508
curl: (26) read function returned funny value
cc

 λ ~ shellcheck tmp 

In tmp line 3:
if [ response=$(curl -F "file=@$PACKAGE" "https://file.io") ]; then
     ^-- SC2077: You need spaces around the comparison operator.
              ^-- SC2046: Quote this to prevent word splitting.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks. Yes, that's worse. We'll keep it as-is for now, I guess. https://stackoverflow.com/a/6934931/3196753

@tresf tresf force-pushed the transfer branch 4 times, most recently from 5330d99 to 030cbd6 Compare December 6, 2017 21:50
@PhysSong
Copy link
Member

PhysSong commented Dec 7, 2017

In respond to failing LADSPA build... @lukas-w disabled C++11 for LADSPA in #4000 and it seems to cause build issues with clang. And -Woverloaded-virtual error seems able to be fixed by simply adding const qualifier.
By the way, why -Werror turned on in Mac build? It was disabled before and I can't find any relevant changes in this PR.

@tresf tresf changed the title Add support for file.io Add support for PR uploads Dec 7, 2017
@tresf
Copy link
Member Author

tresf commented Dec 7, 2017

@PhysSong thanks, I think I'm going to just add the boolean logic back in. This seems like a mistake in #4000.

@lukas-w
Copy link
Member

lukas-w commented Dec 7, 2017

Yes that change was unintentional. Doesn't make sense changing the behaviour in #4000 as it doesn't build with MSVC either way. Sorry about that.

@tresf tresf force-pushed the transfer branch 2 times, most recently from df8027e to 6645e45 Compare December 8, 2017 05:02
@tresf
Copy link
Member Author

tresf commented Dec 8, 2017

Commits squashed.

Note, as part of the Qt5-by-default changes, I've added a new cmake module QtLocator, which aids in fetching the Qt_DIR path for setting up a custom CMAKE_PREFIX_PATH (such as in a non-standard Qt install) but after the cmake process has been fired. It's boilerplate for homebrew/mac now, but can be extended to look for Qt on Windows, Linux as well as QtCreator bundled versions.

The build issues on Apple were due to a bug in script.sh, which overrode -DUSE_WERROR=ON. I've checked and this is explicitly set in win32, win64, linux and osx scripts, so this has been removed from script.sh.

@tresf tresf force-pushed the transfer branch 4 times, most recently from 89d2a45 to a1d367d Compare December 8, 2017 07:20
@tresf tresf mentioned this pull request Dec 8, 2017
7 tasks
@tresf tresf force-pushed the transfer branch 4 times, most recently from a3d068d to 4dcadee Compare December 16, 2017 20:33
@tresf tresf force-pushed the transfer branch 5 times, most recently from 054e202 to 379a1b7 Compare December 23, 2017 03:17
@tresf
Copy link
Member Author

tresf commented Dec 23, 2017

Removes Qt4 from build system
Uploads PRs to transfer.sh
@tresf tresf merged commit 7f9d01e into LMMS:master Dec 23, 2017
@tresf tresf mentioned this pull request Dec 23, 2017
16 tasks
@tresf tresf deleted the transfer branch December 23, 2017 07:30
tresf added a commit that referenced this pull request Jan 2, 2018
Fix a regression caused by #4041, closes #4077
sdasda7777 pushed a commit to sdasda7777/lmms that referenced this pull request Jun 28, 2022
Removes Qt4 from build system
Uploads PRs to transfer.sh
sdasda7777 pushed a commit to sdasda7777/lmms that referenced this pull request Jun 28, 2022
Fix a regression caused by LMMS#4041, closes LMMS#4077
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